* [Qemu-devel] [PULL RESEND 01/19] net: Remove useless local var pkt
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 02/19] eth: Extend vlan stripping functions Jason Wang
` (18 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Fam Zheng, Jason Wang
From: Fam Zheng <famz@redhat.com>
This has been pointless since commit 605d52e62, which was a
search-and-replace, overlooked the redundancy.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/net_rx_pkt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 1019b50..7f928d7 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -159,7 +159,6 @@ void net_rx_pkt_attach_iovec_ex(struct NetRxPkt *pkt,
void net_rx_pkt_dump(struct NetRxPkt *pkt)
{
#ifdef NET_RX_PKT_DEBUG
- NetRxPkt *pkt = (NetRxPkt *)pkt;
assert(pkt);
printf("RX PKT: tot_len: %d, vlan_stripped: %d, vlan_tag: %d\n",
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 02/19] eth: Extend vlan stripping functions
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 01/19] net: Remove useless local var pkt Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 03/19] NetRxPkt: Fix memory corruption on VLAN header stripping Jason Wang
` (17 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Dmitry Fleytman, qemu-stable, Jason Wang
From: Dmitry Fleytman <dmitry@daynix.com>
Make VLAN stripping functions return number of bytes
copied to given Ethernet header buffer.
This information should be used to re-compose
packet IOV after VLAN stripping.
Cc: qemu-stable@nongnu.org
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
include/net/eth.h | 4 ++--
net/eth.c | 25 ++++++++++++++-----------
2 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/include/net/eth.h b/include/net/eth.h
index 2013175..afeb45b 100644
--- a/include/net/eth.h
+++ b/include/net/eth.h
@@ -331,12 +331,12 @@ eth_get_pkt_tci(const void *p)
}
}
-bool
+size_t
eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
uint8_t *new_ehdr_buf,
uint16_t *payload_offset, uint16_t *tci);
-bool
+size_t
eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
uint16_t vet, uint8_t *new_ehdr_buf,
uint16_t *payload_offset, uint16_t *tci);
diff --git a/net/eth.c b/net/eth.c
index df81efb..5b9ba26 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -232,7 +232,7 @@ void eth_get_protocols(const struct iovec *iov, int iovcnt,
}
}
-bool
+size_t
eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
uint8_t *new_ehdr_buf,
uint16_t *payload_offset, uint16_t *tci)
@@ -244,7 +244,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
new_ehdr, sizeof(*new_ehdr));
if (copied < sizeof(*new_ehdr)) {
- return false;
+ return 0;
}
switch (be16_to_cpu(new_ehdr->h_proto)) {
@@ -254,7 +254,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
&vlan_hdr, sizeof(vlan_hdr));
if (copied < sizeof(vlan_hdr)) {
- return false;
+ return 0;
}
new_ehdr->h_proto = vlan_hdr.h_proto;
@@ -268,18 +268,21 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
PKT_GET_VLAN_HDR(new_ehdr), sizeof(vlan_hdr));
if (copied < sizeof(vlan_hdr)) {
- return false;
+ return 0;
}
*payload_offset += sizeof(vlan_hdr);
+
+ return sizeof(struct eth_header) + sizeof(struct vlan_header);
+ } else {
+ return sizeof(struct eth_header);
}
- return true;
default:
- return false;
+ return 0;
}
}
-bool
+size_t
eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
uint16_t vet, uint8_t *new_ehdr_buf,
uint16_t *payload_offset, uint16_t *tci)
@@ -291,7 +294,7 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
new_ehdr, sizeof(*new_ehdr));
if (copied < sizeof(*new_ehdr)) {
- return false;
+ return 0;
}
if (be16_to_cpu(new_ehdr->h_proto) == vet) {
@@ -299,17 +302,17 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
&vlan_hdr, sizeof(vlan_hdr));
if (copied < sizeof(vlan_hdr)) {
- return false;
+ return 0;
}
new_ehdr->h_proto = vlan_hdr.h_proto;
*tci = be16_to_cpu(vlan_hdr.h_tci);
*payload_offset = iovoff + sizeof(*new_ehdr) + sizeof(vlan_hdr);
- return true;
+ return sizeof(struct eth_header);
}
- return false;
+ return 0;
}
void
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 03/19] NetRxPkt: Fix memory corruption on VLAN header stripping
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 01/19] net: Remove useless local var pkt Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 02/19] eth: Extend vlan stripping functions Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 04/19] NetRxPkt: Do not try to pull more data than present Jason Wang
` (16 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Dmitry Fleytman, qemu-stable, Jason Wang
From: Dmitry Fleytman <dmitry@daynix.com>
This patch fixed a problem that was introduced in commit eb700029.
When net_rx_pkt_attach_iovec() calls eth_strip_vlan()
this can result in pkt->ehdr_buf being overflowed, because
ehdr_buf is only sizeof(struct eth_header) bytes large
but eth_strip_vlan() can write
sizeof(struct eth_header) + sizeof(struct vlan_header)
bytes into it.
Devices affected by this problem: vmxnet3.
Cc: qemu-stable@nongnu.org
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/net_rx_pkt.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 7f928d7..3361d7e 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -23,13 +23,13 @@
struct NetRxPkt {
struct virtio_net_hdr virt_hdr;
- uint8_t ehdr_buf[sizeof(struct eth_header)];
+ uint8_t ehdr_buf[sizeof(struct eth_header) + sizeof(struct vlan_header)];
struct iovec *vec;
uint16_t vec_len_total;
uint16_t vec_len;
uint32_t tot_len;
uint16_t tci;
- bool vlan_stripped;
+ size_t ehdr_buf_len;
bool has_virt_hdr;
eth_pkt_types_e packet_type;
@@ -88,15 +88,13 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
const struct iovec *iov, int iovcnt,
size_t ploff)
{
- if (pkt->vlan_stripped) {
+ if (pkt->ehdr_buf_len) {
net_rx_pkt_iovec_realloc(pkt, iovcnt + 1);
pkt->vec[0].iov_base = pkt->ehdr_buf;
- pkt->vec[0].iov_len = sizeof(pkt->ehdr_buf);
-
- pkt->tot_len =
- iov_size(iov, iovcnt) - ploff + sizeof(struct eth_header);
+ pkt->vec[0].iov_len = pkt->ehdr_buf_len;
+ pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len;
pkt->vec_len = iov_copy(pkt->vec + 1, pkt->vec_len_total - 1,
iov, iovcnt, ploff, pkt->tot_len);
} else {
@@ -123,11 +121,12 @@ void net_rx_pkt_attach_iovec(struct NetRxPkt *pkt,
uint16_t tci = 0;
uint16_t ploff = iovoff;
assert(pkt);
- pkt->vlan_stripped = false;
if (strip_vlan) {
- pkt->vlan_stripped = eth_strip_vlan(iov, iovcnt, iovoff, pkt->ehdr_buf,
- &ploff, &tci);
+ pkt->ehdr_buf_len = eth_strip_vlan(iov, iovcnt, iovoff, pkt->ehdr_buf,
+ &ploff, &tci);
+ } else {
+ pkt->ehdr_buf_len = 0;
}
pkt->tci = tci;
@@ -143,12 +142,13 @@ void net_rx_pkt_attach_iovec_ex(struct NetRxPkt *pkt,
uint16_t tci = 0;
uint16_t ploff = iovoff;
assert(pkt);
- pkt->vlan_stripped = false;
if (strip_vlan) {
- pkt->vlan_stripped = eth_strip_vlan_ex(iov, iovcnt, iovoff, vet,
- pkt->ehdr_buf,
- &ploff, &tci);
+ pkt->ehdr_buf_len = eth_strip_vlan_ex(iov, iovcnt, iovoff, vet,
+ pkt->ehdr_buf,
+ &ploff, &tci);
+ } else {
+ pkt->ehdr_buf_len = 0;
}
pkt->tci = tci;
@@ -161,8 +161,8 @@ void net_rx_pkt_dump(struct NetRxPkt *pkt)
#ifdef NET_RX_PKT_DEBUG
assert(pkt);
- printf("RX PKT: tot_len: %d, vlan_stripped: %d, vlan_tag: %d\n",
- pkt->tot_len, pkt->vlan_stripped, pkt->tci);
+ printf("RX PKT: tot_len: %d, ehdr_buf_len: %lu, vlan_tag: %d\n",
+ pkt->tot_len, pkt->ehdr_buf_len, pkt->tci);
#endif
}
@@ -425,7 +425,7 @@ bool net_rx_pkt_is_vlan_stripped(struct NetRxPkt *pkt)
{
assert(pkt);
- return pkt->vlan_stripped;
+ return pkt->ehdr_buf_len ? true : false;
}
bool net_rx_pkt_has_virt_hdr(struct NetRxPkt *pkt)
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 04/19] NetRxPkt: Do not try to pull more data than present
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (2 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 03/19] NetRxPkt: Fix memory corruption on VLAN header stripping Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 05/19] NetRxPkt: Account buffer with ETH header in IOV length Jason Wang
` (15 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Dmitry Fleytman, qemu-stable, Jason Wang
From: Dmitry Fleytman <dmitry@daynix.com>
In case of VLAN stripping, ETH header put into a
separate buffer, therefore amont of data copied
from original IOV should be smaller.
Cc: qemu-stable@nongnu.org
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/net_rx_pkt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 3361d7e..0003a0e 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -96,7 +96,8 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len;
pkt->vec_len = iov_copy(pkt->vec + 1, pkt->vec_len_total - 1,
- iov, iovcnt, ploff, pkt->tot_len);
+ iov, iovcnt, ploff,
+ pkt->tot_len - pkt->ehdr_buf_len);
} else {
net_rx_pkt_iovec_realloc(pkt, iovcnt);
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 05/19] NetRxPkt: Account buffer with ETH header in IOV length
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (3 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 04/19] NetRxPkt: Do not try to pull more data than present Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 06/19] NetRxPkt: Remove code duplication in net_rx_pkt_pull_data() Jason Wang
` (14 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Dmitry Fleytman, qemu-stable, Jason Wang
From: Dmitry Fleytman <dmitry@daynix.com>
In case of VLAN stripping ETH header is stored in a
separate chunk and length of IOV should take this into
account.
This patch fixes checksum validation for RX packets
with VLAN header.
Devices affected by this problem: e1000e and vmxnet3.
Cc: qemu-stable@nongnu.org
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/net_rx_pkt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 0003a0e..2649d40 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -97,7 +97,7 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len;
pkt->vec_len = iov_copy(pkt->vec + 1, pkt->vec_len_total - 1,
iov, iovcnt, ploff,
- pkt->tot_len - pkt->ehdr_buf_len);
+ pkt->tot_len - pkt->ehdr_buf_len) + 1;
} else {
net_rx_pkt_iovec_realloc(pkt, iovcnt);
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 06/19] NetRxPkt: Remove code duplication in net_rx_pkt_pull_data()
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (4 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 05/19] NetRxPkt: Account buffer with ETH header in IOV length Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 07/19] colo-compare: use g_timeout_source_new() to process the stale packets Jason Wang
` (13 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Dmitry Fleytman, Jason Wang
From: Dmitry Fleytman <dmitry@daynix.com>
This is a refactoring commit that does not change behavior.
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/net_rx_pkt.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 2649d40..cef1c2e 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -88,20 +88,21 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
const struct iovec *iov, int iovcnt,
size_t ploff)
{
+ uint32_t pllen = iov_size(iov, iovcnt) - ploff;
+
if (pkt->ehdr_buf_len) {
net_rx_pkt_iovec_realloc(pkt, iovcnt + 1);
pkt->vec[0].iov_base = pkt->ehdr_buf;
pkt->vec[0].iov_len = pkt->ehdr_buf_len;
- pkt->tot_len = iov_size(iov, iovcnt) - ploff + pkt->ehdr_buf_len;
+ pkt->tot_len = pllen + pkt->ehdr_buf_len;
pkt->vec_len = iov_copy(pkt->vec + 1, pkt->vec_len_total - 1,
- iov, iovcnt, ploff,
- pkt->tot_len - pkt->ehdr_buf_len) + 1;
+ iov, iovcnt, ploff, pllen) + 1;
} else {
net_rx_pkt_iovec_realloc(pkt, iovcnt);
- pkt->tot_len = iov_size(iov, iovcnt) - ploff;
+ pkt->tot_len = pllen;
pkt->vec_len = iov_copy(pkt->vec, pkt->vec_len_total,
iov, iovcnt, ploff, pkt->tot_len);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 07/19] colo-compare: use g_timeout_source_new() to process the stale packets
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (5 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 06/19] NetRxPkt: Remove code duplication in net_rx_pkt_pull_data() Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 08/19] colo-compare: kick compare thread to exit after some cleanup in finalization Jason Wang
` (12 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: zhanghailiang, Jason Wang
From: zhanghailiang <zhang.zhanghailiang@huawei.com>
Instead of using qemu timer to process the stale packets,
We re-use the colo compare thread to process these packets
by creating a new timeout coroutine.
Besides, since we process all the same vNIC's net connection/packets
in one thread, it is safe to remove the timer_check_lock.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo-compare.c | 62 +++++++++++++++++++-----------------------------------
1 file changed, 22 insertions(+), 40 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 162fd6a..fdde788 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -83,9 +83,6 @@ typedef struct CompareState {
GHashTable *connection_track_table;
/* compare thread, a thread for each NIC */
QemuThread thread;
- /* Timer used on the primary to find packets that are never matched */
- QEMUTimer *timer;
- QemuMutex timer_check_lock;
} CompareState;
typedef struct CompareClass {
@@ -374,9 +371,7 @@ static void colo_compare_connection(void *opaque, void *user_data)
while (!g_queue_is_empty(&conn->primary_list) &&
!g_queue_is_empty(&conn->secondary_list)) {
- qemu_mutex_lock(&s->timer_check_lock);
pkt = g_queue_pop_tail(&conn->primary_list);
- qemu_mutex_unlock(&s->timer_check_lock);
switch (conn->ip_proto) {
case IPPROTO_TCP:
result = g_queue_find_custom(&conn->secondary_list,
@@ -411,9 +406,7 @@ static void colo_compare_connection(void *opaque, void *user_data)
* until next comparison.
*/
trace_colo_compare_main("packet different");
- qemu_mutex_lock(&s->timer_check_lock);
g_queue_push_tail(&conn->primary_list, pkt);
- qemu_mutex_unlock(&s->timer_check_lock);
/* TODO: colo_notify_checkpoint();*/
break;
}
@@ -486,11 +479,26 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
}
}
+/*
+ * Check old packet regularly so it can watch for any packets
+ * that the secondary hasn't produced equivalents of.
+ */
+static gboolean check_old_packet_regular(void *opaque)
+{
+ CompareState *s = opaque;
+
+ /* if have old packet we will notify checkpoint */
+ colo_old_packet_check(s);
+
+ return TRUE;
+}
+
static void *colo_compare_thread(void *opaque)
{
GMainContext *worker_context;
GMainLoop *compare_loop;
CompareState *s = opaque;
+ GSource *timeout_source;
worker_context = g_main_context_new();
@@ -501,8 +509,15 @@ static void *colo_compare_thread(void *opaque)
compare_loop = g_main_loop_new(worker_context, FALSE);
+ /* To kick any packets that the secondary doesn't match */
+ timeout_source = g_timeout_source_new(REGULAR_PACKET_CHECK_MS);
+ g_source_set_callback(timeout_source,
+ (GSourceFunc)check_old_packet_regular, s, NULL);
+ g_source_attach(timeout_source, worker_context);
+
g_main_loop_run(compare_loop);
+ g_source_unref(timeout_source);
g_main_loop_unref(compare_loop);
g_main_context_unref(worker_context);
return NULL;
@@ -604,26 +619,6 @@ static int find_and_check_chardev(Chardev **chr,
}
/*
- * Check old packet regularly so it can watch for any packets
- * that the secondary hasn't produced equivalents of.
- */
-static void check_old_packet_regular(void *opaque)
-{
- CompareState *s = opaque;
-
- timer_mod(s->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
- REGULAR_PACKET_CHECK_MS);
- /* if have old packet we will notify checkpoint */
- /*
- * TODO: Make timer handler run in compare thread
- * like qemu_chr_add_handlers_full.
- */
- qemu_mutex_lock(&s->timer_check_lock);
- colo_old_packet_check(s);
- qemu_mutex_unlock(&s->timer_check_lock);
-}
-
-/*
* Called from the main thread on the primary
* to setup colo-compare.
*/
@@ -665,7 +660,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize);
g_queue_init(&s->conn_list);
- qemu_mutex_init(&s->timer_check_lock);
s->connection_track_table = g_hash_table_new_full(connection_key_hash,
connection_key_equal,
@@ -678,12 +672,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
QEMU_THREAD_JOINABLE);
compare_id++;
- /* A regular timer to kick any packets that the secondary doesn't match */
- s->timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, /* Only when guest runs */
- check_old_packet_regular, s);
- timer_mod(s->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
- REGULAR_PACKET_CHECK_MS);
-
return;
}
@@ -723,12 +711,6 @@ static void colo_compare_finalize(Object *obj)
qemu_thread_join(&s->thread);
}
- if (s->timer) {
- timer_del(s->timer);
- }
-
- qemu_mutex_destroy(&s->timer_check_lock);
-
g_free(s->pri_indev);
g_free(s->sec_indev);
g_free(s->outdev);
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 08/19] colo-compare: kick compare thread to exit after some cleanup in finalization
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (6 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 07/19] colo-compare: use g_timeout_source_new() to process the stale packets Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 09/19] char: remove the right fd been watched in qemu_chr_fe_set_handlers() Jason Wang
` (11 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: zhanghailiang, Jason Wang
From: zhanghailiang <zhang.zhanghailiang@huawei.com>
We should call g_main_loop_quit() to notify colo compare thread to
exit, Or it will run in g_main_loop_run() forever.
Besides, the finalizing process can't happen in context of colo thread,
it is reasonable to remove the 'if (qemu_thread_is_self(&s->thread))'
branch.
Before compare thead exits, some cleanup works need to be
done, All unhandled packets need to be released and connection_track_table
needs to be freed, or there will be memory leak.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo-compare.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index fdde788..37ce75c 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -83,6 +83,8 @@ typedef struct CompareState {
GHashTable *connection_track_table;
/* compare thread, a thread for each NIC */
QemuThread thread;
+
+ GMainLoop *compare_loop;
} CompareState;
typedef struct CompareClass {
@@ -496,7 +498,6 @@ static gboolean check_old_packet_regular(void *opaque)
static void *colo_compare_thread(void *opaque)
{
GMainContext *worker_context;
- GMainLoop *compare_loop;
CompareState *s = opaque;
GSource *timeout_source;
@@ -507,7 +508,7 @@ static void *colo_compare_thread(void *opaque)
qemu_chr_fe_set_handlers(&s->chr_sec_in, compare_chr_can_read,
compare_sec_chr_in, NULL, s, worker_context, true);
- compare_loop = g_main_loop_new(worker_context, FALSE);
+ s->compare_loop = g_main_loop_new(worker_context, FALSE);
/* To kick any packets that the secondary doesn't match */
timeout_source = g_timeout_source_new(REGULAR_PACKET_CHECK_MS);
@@ -515,10 +516,10 @@ static void *colo_compare_thread(void *opaque)
(GSourceFunc)check_old_packet_regular, s, NULL);
g_source_attach(timeout_source, worker_context);
- g_main_loop_run(compare_loop);
+ g_main_loop_run(s->compare_loop);
g_source_unref(timeout_source);
- g_main_loop_unref(compare_loop);
+ g_main_loop_unref(s->compare_loop);
g_main_context_unref(worker_context);
return NULL;
}
@@ -675,6 +676,23 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
return;
}
+static void colo_flush_packets(void *opaque, void *user_data)
+{
+ CompareState *s = user_data;
+ Connection *conn = opaque;
+ Packet *pkt = NULL;
+
+ while (!g_queue_is_empty(&conn->primary_list)) {
+ pkt = g_queue_pop_head(&conn->primary_list);
+ compare_chr_send(&s->chr_out, pkt->data, pkt->size);
+ packet_destroy(pkt, NULL);
+ }
+ while (!g_queue_is_empty(&conn->secondary_list)) {
+ pkt = g_queue_pop_head(&conn->secondary_list);
+ packet_destroy(pkt, NULL);
+ }
+}
+
static void colo_compare_class_init(ObjectClass *oc, void *data)
{
UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
@@ -703,14 +721,15 @@ static void colo_compare_finalize(Object *obj)
qemu_chr_fe_deinit(&s->chr_sec_in);
qemu_chr_fe_deinit(&s->chr_out);
- g_queue_free(&s->conn_list);
+ g_main_loop_quit(s->compare_loop);
+ qemu_thread_join(&s->thread);
- if (qemu_thread_is_self(&s->thread)) {
- /* compare connection */
- g_queue_foreach(&s->conn_list, colo_compare_connection, s);
- qemu_thread_join(&s->thread);
- }
+ /* Release all unhandled packets after compare thead exited */
+ g_queue_foreach(&s->conn_list, colo_flush_packets, s);
+
+ g_queue_free(&s->conn_list);
+ g_hash_table_destroy(s->connection_track_table);
g_free(s->pri_indev);
g_free(s->sec_indev);
g_free(s->outdev);
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 09/19] char: remove the right fd been watched in qemu_chr_fe_set_handlers()
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (7 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 08/19] colo-compare: kick compare thread to exit after some cleanup in finalization Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 10/19] colo-compare: Fix removing fds been watched incorrectly in finalization Jason Wang
` (10 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel
Cc: zhanghailiang, Paolo Bonzini, Marc-André Lureau, Jason Wang
From: zhanghailiang <zhang.zhanghailiang@huawei.com>
We can call qemu_chr_fe_set_handlers() to add/remove fd been watched
in 'context' which can be either default main context or other explicit
context. But the original logic is not correct, we didn't remove
the right fd because we call g_main_context_find_source_by_id(NULL, tag)
which always try to find the Gsource from default context.
Fix it by passing the right context to g_main_context_find_source_by_id().
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
chardev/char-fd.c | 6 +++---
chardev/char-io.c | 8 ++++----
chardev/char-io.h | 2 +-
chardev/char-pty.c | 2 +-
chardev/char-socket.c | 4 ++--
chardev/char-udp.c | 6 +++---
chardev/char.c | 2 +-
7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index fb51ab4..548dd4c 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -58,7 +58,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
ret = qio_channel_read(
chan, (gchar *)buf, len, NULL);
if (ret == 0) {
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
return FALSE;
}
@@ -89,7 +89,7 @@ static void fd_chr_update_read_handler(Chardev *chr,
{
FDChardev *s = FD_CHARDEV(chr);
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
if (s->ioc_in) {
chr->fd_in_tag = io_add_watch_poll(chr, s->ioc_in,
fd_chr_read_poll,
@@ -103,7 +103,7 @@ static void char_fd_finalize(Object *obj)
Chardev *chr = CHARDEV(obj);
FDChardev *s = FD_CHARDEV(obj);
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
if (s->ioc_in) {
object_unref(OBJECT(s->ioc_in));
}
diff --git a/chardev/char-io.c b/chardev/char-io.c
index 7dfc3f2..b4bb094 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -127,14 +127,14 @@ guint io_add_watch_poll(Chardev *chr,
return tag;
}
-static void io_remove_watch_poll(guint tag)
+static void io_remove_watch_poll(guint tag, GMainContext *context)
{
GSource *source;
IOWatchPoll *iwp;
g_return_if_fail(tag > 0);
- source = g_main_context_find_source_by_id(NULL, tag);
+ source = g_main_context_find_source_by_id(context, tag);
g_return_if_fail(source != NULL);
iwp = io_watch_poll_from_source(source);
@@ -146,10 +146,10 @@ static void io_remove_watch_poll(guint tag)
g_source_destroy(&iwp->parent);
}
-void remove_fd_in_watch(Chardev *chr)
+void remove_fd_in_watch(Chardev *chr, GMainContext *context)
{
if (chr->fd_in_tag) {
- io_remove_watch_poll(chr->fd_in_tag);
+ io_remove_watch_poll(chr->fd_in_tag, context);
chr->fd_in_tag = 0;
}
}
diff --git a/chardev/char-io.h b/chardev/char-io.h
index d7ae5f1..842be56 100644
--- a/chardev/char-io.h
+++ b/chardev/char-io.h
@@ -36,7 +36,7 @@ guint io_add_watch_poll(Chardev *chr,
gpointer user_data,
GMainContext *context);
-void remove_fd_in_watch(Chardev *chr);
+void remove_fd_in_watch(Chardev *chr, GMainContext *context);
int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index ecf2c7a..a6337be 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -199,7 +199,7 @@ static void pty_chr_state(Chardev *chr, int connected)
g_source_remove(s->open_tag);
s->open_tag = 0;
}
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
s->connected = 0;
/* (re-)connect poll interval for idle guests: once per second.
* We check more frequently in case the guests sends data to
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 865c527..d7e92e1 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -328,7 +328,7 @@ static void tcp_chr_free_connection(Chardev *chr)
}
tcp_set_msgfds(chr, NULL, 0);
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
object_unref(OBJECT(s->sioc));
s->sioc = NULL;
object_unref(OBJECT(s->ioc));
@@ -498,7 +498,7 @@ static void tcp_chr_update_read_handler(Chardev *chr,
return;
}
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
if (s->ioc) {
chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
tcp_chr_read_poll,
diff --git a/chardev/char-udp.c b/chardev/char-udp.c
index 2c6c7dd..804bd22 100644
--- a/chardev/char-udp.c
+++ b/chardev/char-udp.c
@@ -81,7 +81,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
ret = qio_channel_read(
s->ioc, (char *)s->buf, sizeof(s->buf), NULL);
if (ret <= 0) {
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
return FALSE;
}
s->bufcnt = ret;
@@ -101,7 +101,7 @@ static void udp_chr_update_read_handler(Chardev *chr,
{
UdpChardev *s = UDP_CHARDEV(chr);
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
if (s->ioc) {
chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
udp_chr_read_poll,
@@ -115,7 +115,7 @@ static void char_udp_finalize(Object *obj)
Chardev *chr = CHARDEV(obj);
UdpChardev *s = UDP_CHARDEV(obj);
- remove_fd_in_watch(chr);
+ remove_fd_in_watch(chr, NULL);
if (s->ioc) {
object_unref(OBJECT(s->ioc));
}
diff --git a/chardev/char.c b/chardev/char.c
index 54cd5f4..3df1163 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -560,7 +560,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
cc = CHARDEV_GET_CLASS(s);
if (!opaque && !fd_can_read && !fd_read && !fd_event) {
fe_open = 0;
- remove_fd_in_watch(s);
+ remove_fd_in_watch(s, context);
} else {
fe_open = 1;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 10/19] colo-compare: Fix removing fds been watched incorrectly in finalization
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (8 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 09/19] char: remove the right fd been watched in qemu_chr_fe_set_handlers() Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 11/19] net/colo-compare: Fix memory free error Jason Wang
` (9 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: zhanghailiang, Jason Wang
From: zhanghailiang <zhang.zhanghailiang@huawei.com>
We will catch the bellow error report while try to delete compare object
by qmp command:
chardev/char-io.c:91: io_watch_poll_finalize: Assertion `iwp->src == ((void *)0)' failed.
This is caused by failing to remove the right fd been watched while
call qemu_chr_fe_set_handlers();
Fix it by pass the worker_context parameter to qemu_chr_fe_set_handlers().
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo-compare.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 37ce75c..a6fc2ff 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -84,6 +84,7 @@ typedef struct CompareState {
/* compare thread, a thread for each NIC */
QemuThread thread;
+ GMainContext *worker_context;
GMainLoop *compare_loop;
} CompareState;
@@ -497,30 +498,29 @@ static gboolean check_old_packet_regular(void *opaque)
static void *colo_compare_thread(void *opaque)
{
- GMainContext *worker_context;
CompareState *s = opaque;
GSource *timeout_source;
- worker_context = g_main_context_new();
+ s->worker_context = g_main_context_new();
qemu_chr_fe_set_handlers(&s->chr_pri_in, compare_chr_can_read,
- compare_pri_chr_in, NULL, s, worker_context, true);
+ compare_pri_chr_in, NULL, s, s->worker_context, true);
qemu_chr_fe_set_handlers(&s->chr_sec_in, compare_chr_can_read,
- compare_sec_chr_in, NULL, s, worker_context, true);
+ compare_sec_chr_in, NULL, s, s->worker_context, true);
- s->compare_loop = g_main_loop_new(worker_context, FALSE);
+ s->compare_loop = g_main_loop_new(s->worker_context, FALSE);
/* To kick any packets that the secondary doesn't match */
timeout_source = g_timeout_source_new(REGULAR_PACKET_CHECK_MS);
g_source_set_callback(timeout_source,
(GSourceFunc)check_old_packet_regular, s, NULL);
- g_source_attach(timeout_source, worker_context);
+ g_source_attach(timeout_source, s->worker_context);
g_main_loop_run(s->compare_loop);
g_source_unref(timeout_source);
g_main_loop_unref(s->compare_loop);
- g_main_context_unref(worker_context);
+ g_main_context_unref(s->worker_context);
return NULL;
}
@@ -717,8 +717,10 @@ static void colo_compare_finalize(Object *obj)
{
CompareState *s = COLO_COMPARE(obj);
- qemu_chr_fe_deinit(&s->chr_pri_in);
- qemu_chr_fe_deinit(&s->chr_sec_in);
+ qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL,
+ s->worker_context, true);
+ qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL,
+ s->worker_context, true);
qemu_chr_fe_deinit(&s->chr_out);
g_main_loop_quit(s->compare_loop);
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 11/19] net/colo-compare: Fix memory free error
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (9 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 10/19] colo-compare: Fix removing fds been watched incorrectly in finalization Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's Jason Wang
` (8 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Zhang Chen, Jason Wang
From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
We use g_queue_init() to init s->conn_list, so we should use g_queue_clear()
to instead of g_queue_free().
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo-compare.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index a6fc2ff..300f017 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -729,7 +729,7 @@ static void colo_compare_finalize(Object *obj)
/* Release all unhandled packets after compare thead exited */
g_queue_foreach(&s->conn_list, colo_flush_packets, s);
- g_queue_free(&s->conn_list);
+ g_queue_clear(&s->conn_list);
g_hash_table_destroy(s->connection_track_table);
g_free(s->pri_indev);
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (10 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 11/19] net/colo-compare: Fix memory free error Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-13 20:20 ` Laurent Vivier
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 13/19] vmxnet3: VMStatify rx/tx q_descr and int_state Jason Wang
` (7 subsequent siblings)
19 siblings, 1 reply; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Dr. David Alan Gilbert, Jason Wang
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
The index's in the Vmxnet3Ring were migrated as 32bit ints
yet are declared as size_t's. They appear to be derived
from 32bit values loaded from guest memory, so actually
store them as that.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Acked-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/vmxnet3.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index e13a798..224c109 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
/* Cyclic ring abstraction */
typedef struct {
hwaddr pa;
- size_t size;
- size_t cell_size;
- size_t next;
+ uint32_t size;
+ uint32_t cell_size;
+ uint32_t next;
uint8_t gen;
} Vmxnet3Ring;
static inline void vmxnet3_ring_init(PCIDevice *d,
Vmxnet3Ring *ring,
hwaddr pa,
- size_t size,
- size_t cell_size,
+ uint32_t size,
+ uint32_t cell_size,
bool zero_region)
{
ring->pa = pa;
@@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
}
#define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
- macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
+ macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
(ring_name), (ridx), \
(r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's Jason Wang
@ 2017-03-13 20:20 ` Laurent Vivier
2017-03-14 11:16 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 31+ messages in thread
From: Laurent Vivier @ 2017-03-13 20:20 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Jason Wang, peter.maydell, qemu-devel
On 06/03/2017 06:25, Jason Wang wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The index's in the Vmxnet3Ring were migrated as 32bit ints
> yet are declared as size_t's. They appear to be derived
> from 32bit values loaded from guest memory, so actually
> store them as that.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> hw/net/vmxnet3.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index e13a798..224c109 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
> /* Cyclic ring abstraction */
> typedef struct {
> hwaddr pa;
> - size_t size;
> - size_t cell_size;
> - size_t next;
> + uint32_t size;
> + uint32_t cell_size;
> + uint32_t next;
> uint8_t gen;
> } Vmxnet3Ring;
>
> static inline void vmxnet3_ring_init(PCIDevice *d,
> Vmxnet3Ring *ring,
> hwaddr pa,
> - size_t size,
> - size_t cell_size,
> + uint32_t size,
> + uint32_t cell_size,
> bool zero_region)
> {
> ring->pa = pa;
> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
> }
>
> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
> (ring_name), (ridx), \
> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
>
>
David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
master I can see the size of "txq_descr" and "rxq_descr" changes because
of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
{
"field": "txq_descr",
"version_id": 0,
"field_exists": false,
"size": 176
},
{
"field": "rxq_descr",
"version_id": 0,
"field_exists": false,
"size": 216
},
becomes:
{
"field": "txq_descr",
"version_id": 0,
"field_exists": false,
"size": 144
},
{
"field": "rxq_descr",
"version_id": 0,
"field_exists": false,
"size": 168
},
And if I try a migration, I have:
qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
|| !n_elems' failed.
Laurent
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-13 20:20 ` Laurent Vivier
@ 2017-03-14 11:16 ` Dr. David Alan Gilbert
2017-03-14 11:29 ` Laurent Vivier
0 siblings, 1 reply; 31+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 11:16 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Jason Wang, peter.maydell, qemu-devel
* Laurent Vivier (lvivier@redhat.com) wrote:
> On 06/03/2017 06:25, Jason Wang wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > The index's in the Vmxnet3Ring were migrated as 32bit ints
> > yet are declared as size_t's. They appear to be derived
> > from 32bit values loaded from guest memory, so actually
> > store them as that.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Acked-by: Dmitry Fleytman <dmitry@daynix.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> > hw/net/vmxnet3.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> > index e13a798..224c109 100644
> > --- a/hw/net/vmxnet3.c
> > +++ b/hw/net/vmxnet3.c
> > @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
> > /* Cyclic ring abstraction */
> > typedef struct {
> > hwaddr pa;
> > - size_t size;
> > - size_t cell_size;
> > - size_t next;
> > + uint32_t size;
> > + uint32_t cell_size;
> > + uint32_t next;
> > uint8_t gen;
> > } Vmxnet3Ring;
> >
> > static inline void vmxnet3_ring_init(PCIDevice *d,
> > Vmxnet3Ring *ring,
> > hwaddr pa,
> > - size_t size,
> > - size_t cell_size,
> > + uint32_t size,
> > + uint32_t cell_size,
> > bool zero_region)
> > {
> > ring->pa = pa;
> > @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
> > }
> >
> > #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
> > - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
> > + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
> > (ring_name), (ridx), \
> > (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
> >
> >
>
>
> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
> master I can see the size of "txq_descr" and "rxq_descr" changes because
> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
>
> {
> "field": "txq_descr",
> "version_id": 0,
> "field_exists": false,
> "size": 176
> },
> {
> "field": "rxq_descr",
> "version_id": 0,
> "field_exists": false,
> "size": 216
> },
>
> becomes:
>
> {
> "field": "txq_descr",
> "version_id": 0,
> "field_exists": false,
> "size": 144
> },
> {
> "field": "rxq_descr",
> "version_id": 0,
> "field_exists": false,
> "size": 168
> },
But the old migration code used to write these fields using qemu_put_be32
(from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
-static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
-{
- qemu_put_be64(f, r->pa);
- qemu_put_be32(f, r->size);
- qemu_put_be32(f, r->cell_size);
- qemu_put_be32(f, r->next);
- qemu_put_byte(f, r->gen);
-}
+static const VMStateDescription vmstate_vmxnet3_ring = {
+ .name = "vmxnet3-ring",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(pa, Vmxnet3Ring),
+ VMSTATE_UINT32(size, Vmxnet3Ring),
+ VMSTATE_UINT32(cell_size, Vmxnet3Ring),
+ VMSTATE_UINT32(next, Vmxnet3Ring),
+ VMSTATE_UINT8(gen, Vmxnet3Ring),
+ VMSTATE_END_OF_LIST()
+ }
};
so they used to be size_t's written with be32 and now they're uint32_t's
written as 32bit - so that should be the same.
I'm not sure i understand where the old txq_descr sizes come from - what
managed to print the 176 number before it was converted to VMState?
> And if I try a migration, I have:
>
> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
> || !n_elems' failed.
Interesting; that's Halil's new assert; I need to look.
Dave
> Laurent
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 11:16 ` Dr. David Alan Gilbert
@ 2017-03-14 11:29 ` Laurent Vivier
2017-03-14 11:38 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 31+ messages in thread
From: Laurent Vivier @ 2017-03-14 11:29 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Jason Wang, peter.maydell, qemu-devel
On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
>> On 06/03/2017 06:25, Jason Wang wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> The index's in the Vmxnet3Ring were migrated as 32bit ints
>>> yet are declared as size_t's. They appear to be derived
>>> from 32bit values loaded from guest memory, so actually
>>> store them as that.
>>>
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>> hw/net/vmxnet3.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index e13a798..224c109 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
>>> /* Cyclic ring abstraction */
>>> typedef struct {
>>> hwaddr pa;
>>> - size_t size;
>>> - size_t cell_size;
>>> - size_t next;
>>> + uint32_t size;
>>> + uint32_t cell_size;
>>> + uint32_t next;
>>> uint8_t gen;
>>> } Vmxnet3Ring;
>>>
>>> static inline void vmxnet3_ring_init(PCIDevice *d,
>>> Vmxnet3Ring *ring,
>>> hwaddr pa,
>>> - size_t size,
>>> - size_t cell_size,
>>> + uint32_t size,
>>> + uint32_t cell_size,
>>> bool zero_region)
>>> {
>>> ring->pa = pa;
>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
>>> }
>>>
>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
>>> (ring_name), (ridx), \
>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
>>>
>>>
>>
>>
>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
>> master I can see the size of "txq_descr" and "rxq_descr" changes because
>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
>>
>> {
>> "field": "txq_descr",
>> "version_id": 0,
>> "field_exists": false,
>> "size": 176
>> },
>> {
>> "field": "rxq_descr",
>> "version_id": 0,
>> "field_exists": false,
>> "size": 216
>> },
>>
>> becomes:
>>
>> {
>> "field": "txq_descr",
>> "version_id": 0,
>> "field_exists": false,
>> "size": 144
>> },
>> {
>> "field": "rxq_descr",
>> "version_id": 0,
>> "field_exists": false,
>> "size": 168
>> },
>
> But the old migration code used to write these fields using qemu_put_be32
> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
>
> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
> -{
> - qemu_put_be64(f, r->pa);
> - qemu_put_be32(f, r->size);
> - qemu_put_be32(f, r->cell_size);
> - qemu_put_be32(f, r->next);
> - qemu_put_byte(f, r->gen);
> -}
>
> +static const VMStateDescription vmstate_vmxnet3_ring = {
> + .name = "vmxnet3-ring",
> + .version_id = 0,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(pa, Vmxnet3Ring),
> + VMSTATE_UINT32(size, Vmxnet3Ring),
> + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
> + VMSTATE_UINT32(next, Vmxnet3Ring),
> + VMSTATE_UINT8(gen, Vmxnet3Ring),
> + VMSTATE_END_OF_LIST()
> + }
> };
>
> so they used to be size_t's written with be32 and now they're uint32_t's
> written as 32bit - so that should be the same.
> I'm not sure i understand where the old txq_descr sizes come from - what
> managed to print the 176 number before it was converted to VMState?
I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
static const VMStateDescription vmstate_vmxnet3_txq_descr = {
.name = "vmxnet3-txq-descr",
.version_id = 0,
.fields = (VMStateField[]) {
VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
Vmxnet3Ring),
VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
Vmxnet3Ring),
VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
vmstate_vmxnet3_tx_stats,
struct UPT1_TxStats),
VMSTATE_END_OF_LIST()
}
};
I will check the '-dump-vmstate' code to see if it's relevant.
>
>> And if I try a migration, I have:
>>
>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
>> || !n_elems' failed.
>
> Interesting; that's Halil's new assert; I need to look.
Can't it be related to the size problem?
Thanks,
Laurent
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 11:29 ` Laurent Vivier
@ 2017-03-14 11:38 ` Dr. David Alan Gilbert
2017-03-14 11:47 ` Laurent Vivier
0 siblings, 1 reply; 31+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 11:38 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Jason Wang, peter.maydell, qemu-devel
* Laurent Vivier (lvivier@redhat.com) wrote:
> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (lvivier@redhat.com) wrote:
> >> On 06/03/2017 06:25, Jason Wang wrote:
> >>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>
> >>> The index's in the Vmxnet3Ring were migrated as 32bit ints
> >>> yet are declared as size_t's. They appear to be derived
> >>> from 32bit values loaded from guest memory, so actually
> >>> store them as that.
> >>>
> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>> ---
> >>> hw/net/vmxnet3.c | 12 ++++++------
> >>> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >>> index e13a798..224c109 100644
> >>> --- a/hw/net/vmxnet3.c
> >>> +++ b/hw/net/vmxnet3.c
> >>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
> >>> /* Cyclic ring abstraction */
> >>> typedef struct {
> >>> hwaddr pa;
> >>> - size_t size;
> >>> - size_t cell_size;
> >>> - size_t next;
> >>> + uint32_t size;
> >>> + uint32_t cell_size;
> >>> + uint32_t next;
> >>> uint8_t gen;
> >>> } Vmxnet3Ring;
> >>>
> >>> static inline void vmxnet3_ring_init(PCIDevice *d,
> >>> Vmxnet3Ring *ring,
> >>> hwaddr pa,
> >>> - size_t size,
> >>> - size_t cell_size,
> >>> + uint32_t size,
> >>> + uint32_t cell_size,
> >>> bool zero_region)
> >>> {
> >>> ring->pa = pa;
> >>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
> >>> }
> >>>
> >>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
> >>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
> >>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
> >>> (ring_name), (ridx), \
> >>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
> >>>
> >>>
> >>
> >>
> >> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
> >> master I can see the size of "txq_descr" and "rxq_descr" changes because
> >> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
> >> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
> >>
> >> {
> >> "field": "txq_descr",
> >> "version_id": 0,
> >> "field_exists": false,
> >> "size": 176
> >> },
> >> {
> >> "field": "rxq_descr",
> >> "version_id": 0,
> >> "field_exists": false,
> >> "size": 216
> >> },
> >>
> >> becomes:
> >>
> >> {
> >> "field": "txq_descr",
> >> "version_id": 0,
> >> "field_exists": false,
> >> "size": 144
> >> },
> >> {
> >> "field": "rxq_descr",
> >> "version_id": 0,
> >> "field_exists": false,
> >> "size": 168
> >> },
> >
> > But the old migration code used to write these fields using qemu_put_be32
> > (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
> >
> > -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
> > -{
> > - qemu_put_be64(f, r->pa);
> > - qemu_put_be32(f, r->size);
> > - qemu_put_be32(f, r->cell_size);
> > - qemu_put_be32(f, r->next);
> > - qemu_put_byte(f, r->gen);
> > -}
> >
> > +static const VMStateDescription vmstate_vmxnet3_ring = {
> > + .name = "vmxnet3-ring",
> > + .version_id = 0,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_UINT64(pa, Vmxnet3Ring),
> > + VMSTATE_UINT32(size, Vmxnet3Ring),
> > + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
> > + VMSTATE_UINT32(next, Vmxnet3Ring),
> > + VMSTATE_UINT8(gen, Vmxnet3Ring),
> > + VMSTATE_END_OF_LIST()
> > + }
> > };
> >
> > so they used to be size_t's written with be32 and now they're uint32_t's
> > written as 32bit - so that should be the same.
> > I'm not sure i understand where the old txq_descr sizes come from - what
> > managed to print the 176 number before it was converted to VMState?
>
> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
>
> static const VMStateDescription vmstate_vmxnet3_txq_descr = {
> .name = "vmxnet3-txq-descr",
> .version_id = 0,
> .fields = (VMStateField[]) {
> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> Vmxnet3Ring),
> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> Vmxnet3Ring),
> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
> vmstate_vmxnet3_tx_stats,
> struct UPT1_TxStats),
> VMSTATE_END_OF_LIST()
> }
> };
>
> I will check the '-dump-vmstate' code to see if it's relevant.
OK, but that sizeof() should never make it onto the wire; all that
teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on
that data.
(And again I'm curious what happened in the version before I committed
that change).
> >
> >> And if I try a migration, I have:
> >>
> >> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
> >> || !n_elems' failed.
> >
> > Interesting; that's Halil's new assert; I need to look.
>
> Can't it be related to the size problem?
It could, I need to see exactly how it's failing; what was the full
command line you used, and with what guest?
Dave
>
> Thanks,
> Laurent
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 11:38 ` Dr. David Alan Gilbert
@ 2017-03-14 11:47 ` Laurent Vivier
2017-03-14 12:32 ` Dr. David Alan Gilbert
` (2 more replies)
0 siblings, 3 replies; 31+ messages in thread
From: Laurent Vivier @ 2017-03-14 11:47 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Jason Wang, peter.maydell, qemu-devel
On 14/03/2017 12:38, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
>> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
>>> * Laurent Vivier (lvivier@redhat.com) wrote:
>>>> On 06/03/2017 06:25, Jason Wang wrote:
>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>
>>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints
>>>>> yet are declared as size_t's. They appear to be derived
>>>>> from 32bit values loaded from guest memory, so actually
>>>>> store them as that.
>>>>>
>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> ---
>>>>> hw/net/vmxnet3.c | 12 ++++++------
>>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>>>> index e13a798..224c109 100644
>>>>> --- a/hw/net/vmxnet3.c
>>>>> +++ b/hw/net/vmxnet3.c
>>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
>>>>> /* Cyclic ring abstraction */
>>>>> typedef struct {
>>>>> hwaddr pa;
>>>>> - size_t size;
>>>>> - size_t cell_size;
>>>>> - size_t next;
>>>>> + uint32_t size;
>>>>> + uint32_t cell_size;
>>>>> + uint32_t next;
>>>>> uint8_t gen;
>>>>> } Vmxnet3Ring;
>>>>>
>>>>> static inline void vmxnet3_ring_init(PCIDevice *d,
>>>>> Vmxnet3Ring *ring,
>>>>> hwaddr pa,
>>>>> - size_t size,
>>>>> - size_t cell_size,
>>>>> + uint32_t size,
>>>>> + uint32_t cell_size,
>>>>> bool zero_region)
>>>>> {
>>>>> ring->pa = pa;
>>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
>>>>> }
>>>>>
>>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
>>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
>>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
>>>>> (ring_name), (ridx), \
>>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
>>>>>
>>>>>
>>>>
>>>>
>>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
>>>> master I can see the size of "txq_descr" and "rxq_descr" changes because
>>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
>>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
>>>>
>>>> {
>>>> "field": "txq_descr",
>>>> "version_id": 0,
>>>> "field_exists": false,
>>>> "size": 176
>>>> },
>>>> {
>>>> "field": "rxq_descr",
>>>> "version_id": 0,
>>>> "field_exists": false,
>>>> "size": 216
>>>> },
>>>>
>>>> becomes:
>>>>
>>>> {
>>>> "field": "txq_descr",
>>>> "version_id": 0,
>>>> "field_exists": false,
>>>> "size": 144
>>>> },
>>>> {
>>>> "field": "rxq_descr",
>>>> "version_id": 0,
>>>> "field_exists": false,
>>>> "size": 168
>>>> },
>>>
>>> But the old migration code used to write these fields using qemu_put_be32
>>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
>>>
>>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
>>> -{
>>> - qemu_put_be64(f, r->pa);
>>> - qemu_put_be32(f, r->size);
>>> - qemu_put_be32(f, r->cell_size);
>>> - qemu_put_be32(f, r->next);
>>> - qemu_put_byte(f, r->gen);
>>> -}
>>>
>>> +static const VMStateDescription vmstate_vmxnet3_ring = {
>>> + .name = "vmxnet3-ring",
>>> + .version_id = 0,
>>> + .fields = (VMStateField[]) {
>>> + VMSTATE_UINT64(pa, Vmxnet3Ring),
>>> + VMSTATE_UINT32(size, Vmxnet3Ring),
>>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
>>> + VMSTATE_UINT32(next, Vmxnet3Ring),
>>> + VMSTATE_UINT8(gen, Vmxnet3Ring),
>>> + VMSTATE_END_OF_LIST()
>>> + }
>>> };
>>>
>>> so they used to be size_t's written with be32 and now they're uint32_t's
>>> written as 32bit - so that should be the same.
>>> I'm not sure i understand where the old txq_descr sizes come from - what
>>> managed to print the 176 number before it was converted to VMState?
>>
>> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
>>
>> static const VMStateDescription vmstate_vmxnet3_txq_descr = {
>> .name = "vmxnet3-txq-descr",
>> .version_id = 0,
>> .fields = (VMStateField[]) {
>> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
>> Vmxnet3Ring),
>> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
>> Vmxnet3Ring),
>> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
>> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
>> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
>> vmstate_vmxnet3_tx_stats,
>> struct UPT1_TxStats),
>> VMSTATE_END_OF_LIST()
>> }
>> };
>>
>> I will check the '-dump-vmstate' code to see if it's relevant.
>
> OK, but that sizeof() should never make it onto the wire; all that
> teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on
> that data.
> (And again I'm curious what happened in the version before I committed
> that change).
>
>>>
>>>> And if I try a migration, I have:
>>>>
>>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
>>>> || !n_elems' failed.
>>>
>>> Interesting; that's Halil's new assert; I need to look.
>>
>> Can't it be related to the size problem?
>
> It could, I need to see exactly how it's failing; what was the full
> command line you used, and with what guest?
Tested with pseries machine type on ppc64le host, no guest started.
qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device
vmxnet3
Laurent
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 11:47 ` Laurent Vivier
@ 2017-03-14 12:32 ` Dr. David Alan Gilbert
2017-03-14 12:42 ` Laurent Vivier
2017-03-14 12:44 ` Dr. David Alan Gilbert
2017-03-14 13:31 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 31+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 12:32 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Jason Wang, peter.maydell, qemu-devel
* Laurent Vivier (lvivier@redhat.com) wrote:
> On 14/03/2017 12:38, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (lvivier@redhat.com) wrote:
> >> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
> >>> * Laurent Vivier (lvivier@redhat.com) wrote:
> >>>> On 06/03/2017 06:25, Jason Wang wrote:
> >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>>>
> >>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints
> >>>>> yet are declared as size_t's. They appear to be derived
> >>>>> from 32bit values loaded from guest memory, so actually
> >>>>> store them as that.
> >>>>>
> >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>> ---
> >>>>> hw/net/vmxnet3.c | 12 ++++++------
> >>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >>>>> index e13a798..224c109 100644
> >>>>> --- a/hw/net/vmxnet3.c
> >>>>> +++ b/hw/net/vmxnet3.c
> >>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
> >>>>> /* Cyclic ring abstraction */
> >>>>> typedef struct {
> >>>>> hwaddr pa;
> >>>>> - size_t size;
> >>>>> - size_t cell_size;
> >>>>> - size_t next;
> >>>>> + uint32_t size;
> >>>>> + uint32_t cell_size;
> >>>>> + uint32_t next;
> >>>>> uint8_t gen;
> >>>>> } Vmxnet3Ring;
> >>>>>
> >>>>> static inline void vmxnet3_ring_init(PCIDevice *d,
> >>>>> Vmxnet3Ring *ring,
> >>>>> hwaddr pa,
> >>>>> - size_t size,
> >>>>> - size_t cell_size,
> >>>>> + uint32_t size,
> >>>>> + uint32_t cell_size,
> >>>>> bool zero_region)
> >>>>> {
> >>>>> ring->pa = pa;
> >>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
> >>>>> }
> >>>>>
> >>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
> >>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
> >>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
> >>>>> (ring_name), (ridx), \
> >>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
> >>>> master I can see the size of "txq_descr" and "rxq_descr" changes because
> >>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
> >>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
> >>>>
> >>>> {
> >>>> "field": "txq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 176
> >>>> },
> >>>> {
> >>>> "field": "rxq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 216
> >>>> },
> >>>>
> >>>> becomes:
> >>>>
> >>>> {
> >>>> "field": "txq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 144
> >>>> },
> >>>> {
> >>>> "field": "rxq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 168
> >>>> },
> >>>
> >>> But the old migration code used to write these fields using qemu_put_be32
> >>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
> >>>
> >>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
> >>> -{
> >>> - qemu_put_be64(f, r->pa);
> >>> - qemu_put_be32(f, r->size);
> >>> - qemu_put_be32(f, r->cell_size);
> >>> - qemu_put_be32(f, r->next);
> >>> - qemu_put_byte(f, r->gen);
> >>> -}
> >>>
> >>> +static const VMStateDescription vmstate_vmxnet3_ring = {
> >>> + .name = "vmxnet3-ring",
> >>> + .version_id = 0,
> >>> + .fields = (VMStateField[]) {
> >>> + VMSTATE_UINT64(pa, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(size, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(next, Vmxnet3Ring),
> >>> + VMSTATE_UINT8(gen, Vmxnet3Ring),
> >>> + VMSTATE_END_OF_LIST()
> >>> + }
> >>> };
> >>>
> >>> so they used to be size_t's written with be32 and now they're uint32_t's
> >>> written as 32bit - so that should be the same.
> >>> I'm not sure i understand where the old txq_descr sizes come from - what
> >>> managed to print the 176 number before it was converted to VMState?
> >>
> >> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
> >>
> >> static const VMStateDescription vmstate_vmxnet3_txq_descr = {
> >> .name = "vmxnet3-txq-descr",
> >> .version_id = 0,
> >> .fields = (VMStateField[]) {
> >> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> >> Vmxnet3Ring),
> >> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> >> Vmxnet3Ring),
> >> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
> >> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
> >> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
> >> vmstate_vmxnet3_tx_stats,
> >> struct UPT1_TxStats),
> >> VMSTATE_END_OF_LIST()
> >> }
> >> };
> >>
> >> I will check the '-dump-vmstate' code to see if it's relevant.
> >
> > OK, but that sizeof() should never make it onto the wire; all that
> > teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on
> > that data.
> > (And again I'm curious what happened in the version before I committed
> > that change).
> >
> >>>
> >>>> And if I try a migration, I have:
> >>>>
> >>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
> >>>> || !n_elems' failed.
> >>>
> >>> Interesting; that's Halil's new assert; I need to look.
> >>
> >> Can't it be related to the size problem?
> >
> > It could, I need to see exactly how it's failing; what was the full
> > command line you used, and with what guest?
>
> Tested with pseries machine type on ppc64le host, no guest started.
>
> qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device
> vmxnet3
OK, I can trigger this on x86 on outgoing migration - the same assert
but in the vmstate_save_state side; for me it's in the older vmxstate_vmxnet3_mcast_list
can you confirm that it's the same structure you're hitting on the load side?
Dave
> Laurent
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 12:32 ` Dr. David Alan Gilbert
@ 2017-03-14 12:42 ` Laurent Vivier
0 siblings, 0 replies; 31+ messages in thread
From: Laurent Vivier @ 2017-03-14 12:42 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Jason Wang, peter.maydell, qemu-devel
On 14/03/2017 13:32, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
>> On 14/03/2017 12:38, Dr. David Alan Gilbert wrote:
>>> * Laurent Vivier (lvivier@redhat.com) wrote:
>>>> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
>>>>> * Laurent Vivier (lvivier@redhat.com) wrote:
>>>>>> On 06/03/2017 06:25, Jason Wang wrote:
>>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>>>
>>>>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints
>>>>>>> yet are declared as size_t's. They appear to be derived
>>>>>>> from 32bit values loaded from guest memory, so actually
>>>>>>> store them as that.
>>>>>>>
>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>> ---
>>>>>>> hw/net/vmxnet3.c | 12 ++++++------
>>>>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>>>>>> index e13a798..224c109 100644
>>>>>>> --- a/hw/net/vmxnet3.c
>>>>>>> +++ b/hw/net/vmxnet3.c
>>>>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
>>>>>>> /* Cyclic ring abstraction */
>>>>>>> typedef struct {
>>>>>>> hwaddr pa;
>>>>>>> - size_t size;
>>>>>>> - size_t cell_size;
>>>>>>> - size_t next;
>>>>>>> + uint32_t size;
>>>>>>> + uint32_t cell_size;
>>>>>>> + uint32_t next;
>>>>>>> uint8_t gen;
>>>>>>> } Vmxnet3Ring;
>>>>>>>
>>>>>>> static inline void vmxnet3_ring_init(PCIDevice *d,
>>>>>>> Vmxnet3Ring *ring,
>>>>>>> hwaddr pa,
>>>>>>> - size_t size,
>>>>>>> - size_t cell_size,
>>>>>>> + uint32_t size,
>>>>>>> + uint32_t cell_size,
>>>>>>> bool zero_region)
>>>>>>> {
>>>>>>> ring->pa = pa;
>>>>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
>>>>>>> }
>>>>>>>
>>>>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
>>>>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
>>>>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
>>>>>>> (ring_name), (ridx), \
>>>>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
>>>>>> master I can see the size of "txq_descr" and "rxq_descr" changes because
>>>>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
>>>>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
>>>>>>
>>>>>> {
>>>>>> "field": "txq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 176
>>>>>> },
>>>>>> {
>>>>>> "field": "rxq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 216
>>>>>> },
>>>>>>
>>>>>> becomes:
>>>>>>
>>>>>> {
>>>>>> "field": "txq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 144
>>>>>> },
>>>>>> {
>>>>>> "field": "rxq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 168
>>>>>> },
>>>>>
>>>>> But the old migration code used to write these fields using qemu_put_be32
>>>>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
>>>>>
>>>>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
>>>>> -{
>>>>> - qemu_put_be64(f, r->pa);
>>>>> - qemu_put_be32(f, r->size);
>>>>> - qemu_put_be32(f, r->cell_size);
>>>>> - qemu_put_be32(f, r->next);
>>>>> - qemu_put_byte(f, r->gen);
>>>>> -}
>>>>>
>>>>> +static const VMStateDescription vmstate_vmxnet3_ring = {
>>>>> + .name = "vmxnet3-ring",
>>>>> + .version_id = 0,
>>>>> + .fields = (VMStateField[]) {
>>>>> + VMSTATE_UINT64(pa, Vmxnet3Ring),
>>>>> + VMSTATE_UINT32(size, Vmxnet3Ring),
>>>>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
>>>>> + VMSTATE_UINT32(next, Vmxnet3Ring),
>>>>> + VMSTATE_UINT8(gen, Vmxnet3Ring),
>>>>> + VMSTATE_END_OF_LIST()
>>>>> + }
>>>>> };
>>>>>
>>>>> so they used to be size_t's written with be32 and now they're uint32_t's
>>>>> written as 32bit - so that should be the same.
>>>>> I'm not sure i understand where the old txq_descr sizes come from - what
>>>>> managed to print the 176 number before it was converted to VMState?
>>>>
>>>> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
>>>>
>>>> static const VMStateDescription vmstate_vmxnet3_txq_descr = {
>>>> .name = "vmxnet3-txq-descr",
>>>> .version_id = 0,
>>>> .fields = (VMStateField[]) {
>>>> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
>>>> Vmxnet3Ring),
>>>> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
>>>> Vmxnet3Ring),
>>>> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
>>>> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
>>>> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
>>>> vmstate_vmxnet3_tx_stats,
>>>> struct UPT1_TxStats),
>>>> VMSTATE_END_OF_LIST()
>>>> }
>>>> };
>>>>
>>>> I will check the '-dump-vmstate' code to see if it's relevant.
>>>
>>> OK, but that sizeof() should never make it onto the wire; all that
>>> teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on
>>> that data.
>>> (And again I'm curious what happened in the version before I committed
>>> that change).
>>>
>>>>>
>>>>>> And if I try a migration, I have:
>>>>>>
>>>>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
>>>>>> || !n_elems' failed.
>>>>>
>>>>> Interesting; that's Halil's new assert; I need to look.
>>>>
>>>> Can't it be related to the size problem?
>>>
>>> It could, I need to see exactly how it's failing; what was the full
>>> command line you used, and with what guest?
>>
>> Tested with pseries machine type on ppc64le host, no guest started.
>>
>> qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device
>> vmxnet3
>
> OK, I can trigger this on x86 on outgoing migration - the same assert
> but in the vmstate_save_state side; for me it's in the older vmxstate_vmxnet3_mcast_list
> can you confirm that it's the same structure you're hitting on the load side?
It always happens on the master branch side (load or save):
if master is on the destination side:
#4 0x00000000277fb9e8 in vmstate_load_state (f=0x2b9a2000,
vmsd=0x27d37c08 <vmxstate_vmxnet3_mcast_list>, opaque=0x28a9c000,
version_id=<optimized out>)
at /home/lvivier/Projects/qemu/migration/vmstate.c:112
if master is on the source side:
#4 0x000000002e2bcbb8 in vmstate_save_state (f=0x32462000,
vmsd=0x2e7f7c08 <vmxstate_vmxnet3_mcast_list>, opaque=0x2f55c000,
vmdesc=0x32259ef0) at
/home/lvivier/Projects/qemu/migration/vmstate.c:328
Laurent
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 11:47 ` Laurent Vivier
2017-03-14 12:32 ` Dr. David Alan Gilbert
@ 2017-03-14 12:44 ` Dr. David Alan Gilbert
2017-03-14 13:31 ` Dr. David Alan Gilbert
2 siblings, 0 replies; 31+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 12:44 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Jason Wang, peter.maydell, qemu-devel, pasic
* Laurent Vivier (lvivier@redhat.com) wrote:
> On 14/03/2017 12:38, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (lvivier@redhat.com) wrote:
> >> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
> >>> * Laurent Vivier (lvivier@redhat.com) wrote:
> >>>> On 06/03/2017 06:25, Jason Wang wrote:
> >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>>>
> >>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints
> >>>>> yet are declared as size_t's. They appear to be derived
> >>>>> from 32bit values loaded from guest memory, so actually
> >>>>> store them as that.
> >>>>>
> >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>> ---
> >>>>> hw/net/vmxnet3.c | 12 ++++++------
> >>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >>>>> index e13a798..224c109 100644
> >>>>> --- a/hw/net/vmxnet3.c
> >>>>> +++ b/hw/net/vmxnet3.c
> >>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
> >>>>> /* Cyclic ring abstraction */
> >>>>> typedef struct {
> >>>>> hwaddr pa;
> >>>>> - size_t size;
> >>>>> - size_t cell_size;
> >>>>> - size_t next;
> >>>>> + uint32_t size;
> >>>>> + uint32_t cell_size;
> >>>>> + uint32_t next;
> >>>>> uint8_t gen;
> >>>>> } Vmxnet3Ring;
> >>>>>
> >>>>> static inline void vmxnet3_ring_init(PCIDevice *d,
> >>>>> Vmxnet3Ring *ring,
> >>>>> hwaddr pa,
> >>>>> - size_t size,
> >>>>> - size_t cell_size,
> >>>>> + uint32_t size,
> >>>>> + uint32_t cell_size,
> >>>>> bool zero_region)
> >>>>> {
> >>>>> ring->pa = pa;
> >>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
> >>>>> }
> >>>>>
> >>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
> >>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
> >>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
> >>>>> (ring_name), (ridx), \
> >>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
> >>>> master I can see the size of "txq_descr" and "rxq_descr" changes because
> >>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
> >>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
> >>>>
> >>>> {
> >>>> "field": "txq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 176
> >>>> },
> >>>> {
> >>>> "field": "rxq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 216
> >>>> },
> >>>>
> >>>> becomes:
> >>>>
> >>>> {
> >>>> "field": "txq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 144
> >>>> },
> >>>> {
> >>>> "field": "rxq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 168
> >>>> },
> >>>
> >>> But the old migration code used to write these fields using qemu_put_be32
> >>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
> >>>
> >>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
> >>> -{
> >>> - qemu_put_be64(f, r->pa);
> >>> - qemu_put_be32(f, r->size);
> >>> - qemu_put_be32(f, r->cell_size);
> >>> - qemu_put_be32(f, r->next);
> >>> - qemu_put_byte(f, r->gen);
> >>> -}
> >>>
> >>> +static const VMStateDescription vmstate_vmxnet3_ring = {
> >>> + .name = "vmxnet3-ring",
> >>> + .version_id = 0,
> >>> + .fields = (VMStateField[]) {
> >>> + VMSTATE_UINT64(pa, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(size, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(next, Vmxnet3Ring),
> >>> + VMSTATE_UINT8(gen, Vmxnet3Ring),
> >>> + VMSTATE_END_OF_LIST()
> >>> + }
> >>> };
> >>>
> >>> so they used to be size_t's written with be32 and now they're uint32_t's
> >>> written as 32bit - so that should be the same.
> >>> I'm not sure i understand where the old txq_descr sizes come from - what
> >>> managed to print the 176 number before it was converted to VMState?
> >>
> >> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
> >>
> >> static const VMStateDescription vmstate_vmxnet3_txq_descr = {
> >> .name = "vmxnet3-txq-descr",
> >> .version_id = 0,
> >> .fields = (VMStateField[]) {
> >> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> >> Vmxnet3Ring),
> >> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> >> Vmxnet3Ring),
> >> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
> >> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
> >> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
> >> vmstate_vmxnet3_tx_stats,
> >> struct UPT1_TxStats),
> >> VMSTATE_END_OF_LIST()
> >> }
> >> };
> >>
> >> I will check the '-dump-vmstate' code to see if it's relevant.
> >
> > OK, but that sizeof() should never make it onto the wire; all that
> > teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on
> > that data.
> > (And again I'm curious what happened in the version before I committed
> > that change).
> >
> >>>
> >>>> And if I try a migration, I have:
> >>>>
> >>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
> >>>> || !n_elems' failed.
> >>>
> >>> Interesting; that's Halil's new assert; I need to look.
> >>
> >> Can't it be related to the size problem?
> >
> > It could, I need to see exactly how it's failing; what was the full
> > command line you used, and with what guest?
>
> Tested with pseries machine type on ppc64le host, no guest started.
>
> qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device
> vmxnet3
>
OK, I think this is just Halil's assert being overly-pessimistic, it needs
a similar fix as proposed for s390x;
static const VMStateDescription vmxstate_vmxnet3_mcast_list = {
.name = "vmxnet3/mcast_list",
.version_id = 1,
.minimum_version_id = 1,
.pre_load = vmxnet3_mcast_list_pre_load,
.needed = vmxnet3_mc_list_needed,
.fields = (VMStateField[]) {
VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL,
mcast_list_buff_size),
VMSTATE_END_OF_LIST()
}
};
in the case where the mcast list is empty, we end up with
mcast_list_buff_size =0 and mcast_list = NULL
and I think a VMSTATE_VBUFFER_UINT32 ends up with
first_elem = NULL,
n_elems = 1
*size_offset = 0
so it ends up with a single zero size offset element
at a NULL pointer; which is legal but hits that new assert.
Dave
> Laurent
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 11:47 ` Laurent Vivier
2017-03-14 12:32 ` Dr. David Alan Gilbert
2017-03-14 12:44 ` Dr. David Alan Gilbert
@ 2017-03-14 13:31 ` Dr. David Alan Gilbert
2017-03-14 13:42 ` Laurent Vivier
2 siblings, 1 reply; 31+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 13:31 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Jason Wang, peter.maydell, qemu-devel, haoqf
* Laurent Vivier (lvivier@redhat.com) wrote:
> On 14/03/2017 12:38, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (lvivier@redhat.com) wrote:
> >> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
> >>> * Laurent Vivier (lvivier@redhat.com) wrote:
> >>>> On 06/03/2017 06:25, Jason Wang wrote:
> >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>>>
> >>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints
> >>>>> yet are declared as size_t's. They appear to be derived
> >>>>> from 32bit values loaded from guest memory, so actually
> >>>>> store them as that.
> >>>>>
> >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>> ---
> >>>>> hw/net/vmxnet3.c | 12 ++++++------
> >>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >>>>> index e13a798..224c109 100644
> >>>>> --- a/hw/net/vmxnet3.c
> >>>>> +++ b/hw/net/vmxnet3.c
> >>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
> >>>>> /* Cyclic ring abstraction */
> >>>>> typedef struct {
> >>>>> hwaddr pa;
> >>>>> - size_t size;
> >>>>> - size_t cell_size;
> >>>>> - size_t next;
> >>>>> + uint32_t size;
> >>>>> + uint32_t cell_size;
> >>>>> + uint32_t next;
> >>>>> uint8_t gen;
> >>>>> } Vmxnet3Ring;
> >>>>>
> >>>>> static inline void vmxnet3_ring_init(PCIDevice *d,
> >>>>> Vmxnet3Ring *ring,
> >>>>> hwaddr pa,
> >>>>> - size_t size,
> >>>>> - size_t cell_size,
> >>>>> + uint32_t size,
> >>>>> + uint32_t cell_size,
> >>>>> bool zero_region)
> >>>>> {
> >>>>> ring->pa = pa;
> >>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
> >>>>> }
> >>>>>
> >>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
> >>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
> >>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
> >>>>> (ring_name), (ridx), \
> >>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
> >>>> master I can see the size of "txq_descr" and "rxq_descr" changes because
> >>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
> >>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
> >>>>
> >>>> {
> >>>> "field": "txq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 176
> >>>> },
> >>>> {
> >>>> "field": "rxq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 216
> >>>> },
> >>>>
> >>>> becomes:
> >>>>
> >>>> {
> >>>> "field": "txq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 144
> >>>> },
> >>>> {
> >>>> "field": "rxq_descr",
> >>>> "version_id": 0,
> >>>> "field_exists": false,
> >>>> "size": 168
> >>>> },
> >>>
> >>> But the old migration code used to write these fields using qemu_put_be32
> >>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
> >>>
> >>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
> >>> -{
> >>> - qemu_put_be64(f, r->pa);
> >>> - qemu_put_be32(f, r->size);
> >>> - qemu_put_be32(f, r->cell_size);
> >>> - qemu_put_be32(f, r->next);
> >>> - qemu_put_byte(f, r->gen);
> >>> -}
> >>>
> >>> +static const VMStateDescription vmstate_vmxnet3_ring = {
> >>> + .name = "vmxnet3-ring",
> >>> + .version_id = 0,
> >>> + .fields = (VMStateField[]) {
> >>> + VMSTATE_UINT64(pa, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(size, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
> >>> + VMSTATE_UINT32(next, Vmxnet3Ring),
> >>> + VMSTATE_UINT8(gen, Vmxnet3Ring),
> >>> + VMSTATE_END_OF_LIST()
> >>> + }
> >>> };
> >>>
> >>> so they used to be size_t's written with be32 and now they're uint32_t's
> >>> written as 32bit - so that should be the same.
> >>> I'm not sure i understand where the old txq_descr sizes come from - what
> >>> managed to print the 176 number before it was converted to VMState?
> >>
> >> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
> >>
> >> static const VMStateDescription vmstate_vmxnet3_txq_descr = {
> >> .name = "vmxnet3-txq-descr",
> >> .version_id = 0,
> >> .fields = (VMStateField[]) {
> >> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> >> Vmxnet3Ring),
> >> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
> >> Vmxnet3Ring),
> >> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
> >> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
> >> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
> >> vmstate_vmxnet3_tx_stats,
> >> struct UPT1_TxStats),
> >> VMSTATE_END_OF_LIST()
> >> }
> >> };
> >>
> >> I will check the '-dump-vmstate' code to see if it's relevant.
> >
> > OK, but that sizeof() should never make it onto the wire; all that
> > teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on
> > that data.
> > (And again I'm curious what happened in the version before I committed
> > that change).
> >
> >>>
> >>>> And if I try a migration, I have:
> >>>>
> >>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
> >>>> || !n_elems' failed.
> >>>
> >>> Interesting; that's Halil's new assert; I need to look.
> >>
> >> Can't it be related to the size problem?
> >
> > It could, I need to see exactly how it's failing; what was the full
> > command line you used, and with what guest?
>
> Tested with pseries machine type on ppc64le host, no guest started.
>
> qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device
> vmxnet3
QingFeng Hao's <haoqf@linux.vnet.ibm.com> patch:
'vmstate: fix failed iotests case 68 and 91'
seems to fix that for me.
Dave
> Laurent
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's
2017-03-14 13:31 ` Dr. David Alan Gilbert
@ 2017-03-14 13:42 ` Laurent Vivier
0 siblings, 0 replies; 31+ messages in thread
From: Laurent Vivier @ 2017-03-14 13:42 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Jason Wang, peter.maydell, qemu-devel, haoqf
On 14/03/2017 14:31, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
>> On 14/03/2017 12:38, Dr. David Alan Gilbert wrote:
>>> * Laurent Vivier (lvivier@redhat.com) wrote:
>>>> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
>>>>> * Laurent Vivier (lvivier@redhat.com) wrote:
>>>>>> On 06/03/2017 06:25, Jason Wang wrote:
>>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>>>
>>>>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints
>>>>>>> yet are declared as size_t's. They appear to be derived
>>>>>>> from 32bit values loaded from guest memory, so actually
>>>>>>> store them as that.
>>>>>>>
>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com>
>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>> ---
>>>>>>> hw/net/vmxnet3.c | 12 ++++++------
>>>>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>>>>>> index e13a798..224c109 100644
>>>>>>> --- a/hw/net/vmxnet3.c
>>>>>>> +++ b/hw/net/vmxnet3.c
>>>>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class {
>>>>>>> /* Cyclic ring abstraction */
>>>>>>> typedef struct {
>>>>>>> hwaddr pa;
>>>>>>> - size_t size;
>>>>>>> - size_t cell_size;
>>>>>>> - size_t next;
>>>>>>> + uint32_t size;
>>>>>>> + uint32_t cell_size;
>>>>>>> + uint32_t next;
>>>>>>> uint8_t gen;
>>>>>>> } Vmxnet3Ring;
>>>>>>>
>>>>>>> static inline void vmxnet3_ring_init(PCIDevice *d,
>>>>>>> Vmxnet3Ring *ring,
>>>>>>> hwaddr pa,
>>>>>>> - size_t size,
>>>>>>> - size_t cell_size,
>>>>>>> + uint32_t size,
>>>>>>> + uint32_t cell_size,
>>>>>>> bool zero_region)
>>>>>>> {
>>>>>>> ring->pa = pa;
>>>>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d,
>>>>>>> }
>>>>>>>
>>>>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \
>>>>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \
>>>>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \
>>>>>>> (ring_name), (ridx), \
>>>>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and
>>>>>> master I can see the size of "txq_descr" and "rxq_descr" changes because
>>>>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes
>>>>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/":
>>>>>>
>>>>>> {
>>>>>> "field": "txq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 176
>>>>>> },
>>>>>> {
>>>>>> "field": "rxq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 216
>>>>>> },
>>>>>>
>>>>>> becomes:
>>>>>>
>>>>>> {
>>>>>> "field": "txq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 144
>>>>>> },
>>>>>> {
>>>>>> "field": "rxq_descr",
>>>>>> "version_id": 0,
>>>>>> "field_exists": false,
>>>>>> "size": 168
>>>>>> },
>>>>>
>>>>> But the old migration code used to write these fields using qemu_put_be32
>>>>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3)
>>>>>
>>>>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
>>>>> -{
>>>>> - qemu_put_be64(f, r->pa);
>>>>> - qemu_put_be32(f, r->size);
>>>>> - qemu_put_be32(f, r->cell_size);
>>>>> - qemu_put_be32(f, r->next);
>>>>> - qemu_put_byte(f, r->gen);
>>>>> -}
>>>>>
>>>>> +static const VMStateDescription vmstate_vmxnet3_ring = {
>>>>> + .name = "vmxnet3-ring",
>>>>> + .version_id = 0,
>>>>> + .fields = (VMStateField[]) {
>>>>> + VMSTATE_UINT64(pa, Vmxnet3Ring),
>>>>> + VMSTATE_UINT32(size, Vmxnet3Ring),
>>>>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring),
>>>>> + VMSTATE_UINT32(next, Vmxnet3Ring),
>>>>> + VMSTATE_UINT8(gen, Vmxnet3Ring),
>>>>> + VMSTATE_END_OF_LIST()
>>>>> + }
>>>>> };
>>>>>
>>>>> so they used to be size_t's written with be32 and now they're uint32_t's
>>>>> written as 32bit - so that should be the same.
>>>>> I'm not sure i understand where the old txq_descr sizes come from - what
>>>>> managed to print the 176 number before it was converted to VMState?
>>>>
>>>> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr):
>>>>
>>>> static const VMStateDescription vmstate_vmxnet3_txq_descr = {
>>>> .name = "vmxnet3-txq-descr",
>>>> .version_id = 0,
>>>> .fields = (VMStateField[]) {
>>>> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
>>>> Vmxnet3Ring),
>>>> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
>>>> Vmxnet3Ring),
>>>> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
>>>> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
>>>> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0,
>>>> vmstate_vmxnet3_tx_stats,
>>>> struct UPT1_TxStats),
>>>> VMSTATE_END_OF_LIST()
>>>> }
>>>> };
>>>>
>>>> I will check the '-dump-vmstate' code to see if it's relevant.
>>>
>>> OK, but that sizeof() should never make it onto the wire; all that
>>> teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on
>>> that data.
>>> (And again I'm curious what happened in the version before I committed
>>> that change).
>>>
>>>>>
>>>>>> And if I try a migration, I have:
>>>>>>
>>>>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem
>>>>>> || !n_elems' failed.
>>>>>
>>>>> Interesting; that's Halil's new assert; I need to look.
>>>>
>>>> Can't it be related to the size problem?
>>>
>>> It could, I need to see exactly how it's failing; what was the full
>>> command line you used, and with what guest?
>>
>> Tested with pseries machine type on ppc64le host, no guest started.
>>
>> qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device
>> vmxnet3
>
> QingFeng Hao's <haoqf@linux.vnet.ibm.com> patch:
> 'vmstate: fix failed iotests case 68 and 91'
> seems to fix that for me.
For me too.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PULL RESEND 13/19] vmxnet3: VMStatify rx/tx q_descr and int_state
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (11 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 14/19] net/colo: fix memory double free error Jason Wang
` (6 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Dr. David Alan Gilbert, Jason Wang
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Fairly simple mechanical conversion of all fields.
TODO!!!!
The problem is vmxnet3-ring size/cell_size/next are declared as size_t
but written as 32bit.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Acked-by: Dmitry Fleytman <dmitry@daynix.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/vmxnet3.c | 272 ++++++++++++++++++-------------------------------------
1 file changed, 90 insertions(+), 182 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 224c109..8b1fab2 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2403,155 +2403,87 @@ static const VMStateDescription vmxstate_vmxnet3_mcast_list = {
}
};
-static void vmxnet3_get_ring_from_file(QEMUFile *f, Vmxnet3Ring *r)
-{
- r->pa = qemu_get_be64(f);
- r->size = qemu_get_be32(f);
- r->cell_size = qemu_get_be32(f);
- r->next = qemu_get_be32(f);
- r->gen = qemu_get_byte(f);
-}
-
-static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r)
-{
- qemu_put_be64(f, r->pa);
- qemu_put_be32(f, r->size);
- qemu_put_be32(f, r->cell_size);
- qemu_put_be32(f, r->next);
- qemu_put_byte(f, r->gen);
-}
-
-static void vmxnet3_get_tx_stats_from_file(QEMUFile *f,
- struct UPT1_TxStats *tx_stat)
-{
- tx_stat->TSOPktsTxOK = qemu_get_be64(f);
- tx_stat->TSOBytesTxOK = qemu_get_be64(f);
- tx_stat->ucastPktsTxOK = qemu_get_be64(f);
- tx_stat->ucastBytesTxOK = qemu_get_be64(f);
- tx_stat->mcastPktsTxOK = qemu_get_be64(f);
- tx_stat->mcastBytesTxOK = qemu_get_be64(f);
- tx_stat->bcastPktsTxOK = qemu_get_be64(f);
- tx_stat->bcastBytesTxOK = qemu_get_be64(f);
- tx_stat->pktsTxError = qemu_get_be64(f);
- tx_stat->pktsTxDiscard = qemu_get_be64(f);
-}
-
-static void vmxnet3_put_tx_stats_to_file(QEMUFile *f,
- struct UPT1_TxStats *tx_stat)
-{
- qemu_put_be64(f, tx_stat->TSOPktsTxOK);
- qemu_put_be64(f, tx_stat->TSOBytesTxOK);
- qemu_put_be64(f, tx_stat->ucastPktsTxOK);
- qemu_put_be64(f, tx_stat->ucastBytesTxOK);
- qemu_put_be64(f, tx_stat->mcastPktsTxOK);
- qemu_put_be64(f, tx_stat->mcastBytesTxOK);
- qemu_put_be64(f, tx_stat->bcastPktsTxOK);
- qemu_put_be64(f, tx_stat->bcastBytesTxOK);
- qemu_put_be64(f, tx_stat->pktsTxError);
- qemu_put_be64(f, tx_stat->pktsTxDiscard);
-}
-
-static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size,
- VMStateField *field)
-{
- Vmxnet3TxqDescr *r = pv;
-
- vmxnet3_get_ring_from_file(f, &r->tx_ring);
- vmxnet3_get_ring_from_file(f, &r->comp_ring);
- r->intr_idx = qemu_get_byte(f);
- r->tx_stats_pa = qemu_get_be64(f);
-
- vmxnet3_get_tx_stats_from_file(f, &r->txq_stats);
-
- return 0;
-}
-
-static int vmxnet3_put_txq_descr(QEMUFile *f, void *pv, size_t size,
- VMStateField *field, QJSON *vmdesc)
-{
- Vmxnet3TxqDescr *r = pv;
-
- vmxnet3_put_ring_to_file(f, &r->tx_ring);
- vmxnet3_put_ring_to_file(f, &r->comp_ring);
- qemu_put_byte(f, r->intr_idx);
- qemu_put_be64(f, r->tx_stats_pa);
- vmxnet3_put_tx_stats_to_file(f, &r->txq_stats);
-
- return 0;
-}
-
-static const VMStateInfo txq_descr_info = {
- .name = "txq_descr",
- .get = vmxnet3_get_txq_descr,
- .put = vmxnet3_put_txq_descr
+static const VMStateDescription vmstate_vmxnet3_ring = {
+ .name = "vmxnet3-ring",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(pa, Vmxnet3Ring),
+ VMSTATE_UINT32(size, Vmxnet3Ring),
+ VMSTATE_UINT32(cell_size, Vmxnet3Ring),
+ VMSTATE_UINT32(next, Vmxnet3Ring),
+ VMSTATE_UINT8(gen, Vmxnet3Ring),
+ VMSTATE_END_OF_LIST()
+ }
};
-static void vmxnet3_get_rx_stats_from_file(QEMUFile *f,
- struct UPT1_RxStats *rx_stat)
-{
- rx_stat->LROPktsRxOK = qemu_get_be64(f);
- rx_stat->LROBytesRxOK = qemu_get_be64(f);
- rx_stat->ucastPktsRxOK = qemu_get_be64(f);
- rx_stat->ucastBytesRxOK = qemu_get_be64(f);
- rx_stat->mcastPktsRxOK = qemu_get_be64(f);
- rx_stat->mcastBytesRxOK = qemu_get_be64(f);
- rx_stat->bcastPktsRxOK = qemu_get_be64(f);
- rx_stat->bcastBytesRxOK = qemu_get_be64(f);
- rx_stat->pktsRxOutOfBuf = qemu_get_be64(f);
- rx_stat->pktsRxError = qemu_get_be64(f);
-}
-
-static void vmxnet3_put_rx_stats_to_file(QEMUFile *f,
- struct UPT1_RxStats *rx_stat)
-{
- qemu_put_be64(f, rx_stat->LROPktsRxOK);
- qemu_put_be64(f, rx_stat->LROBytesRxOK);
- qemu_put_be64(f, rx_stat->ucastPktsRxOK);
- qemu_put_be64(f, rx_stat->ucastBytesRxOK);
- qemu_put_be64(f, rx_stat->mcastPktsRxOK);
- qemu_put_be64(f, rx_stat->mcastBytesRxOK);
- qemu_put_be64(f, rx_stat->bcastPktsRxOK);
- qemu_put_be64(f, rx_stat->bcastBytesRxOK);
- qemu_put_be64(f, rx_stat->pktsRxOutOfBuf);
- qemu_put_be64(f, rx_stat->pktsRxError);
-}
-
-static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size,
- VMStateField *field)
-{
- Vmxnet3RxqDescr *r = pv;
- int i;
-
- for (i = 0; i < VMXNET3_RX_RINGS_PER_QUEUE; i++) {
- vmxnet3_get_ring_from_file(f, &r->rx_ring[i]);
+static const VMStateDescription vmstate_vmxnet3_tx_stats = {
+ .name = "vmxnet3-tx-stats",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(TSOPktsTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(TSOBytesTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(ucastPktsTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(ucastBytesTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(mcastPktsTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(mcastBytesTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(bcastPktsTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(bcastBytesTxOK, struct UPT1_TxStats),
+ VMSTATE_UINT64(pktsTxError, struct UPT1_TxStats),
+ VMSTATE_UINT64(pktsTxDiscard, struct UPT1_TxStats),
+ VMSTATE_END_OF_LIST()
}
+};
- vmxnet3_get_ring_from_file(f, &r->comp_ring);
- r->intr_idx = qemu_get_byte(f);
- r->rx_stats_pa = qemu_get_be64(f);
-
- vmxnet3_get_rx_stats_from_file(f, &r->rxq_stats);
-
- return 0;
-}
-
-static int vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size,
- VMStateField *field, QJSON *vmdesc)
-{
- Vmxnet3RxqDescr *r = pv;
- int i;
-
- for (i = 0; i < VMXNET3_RX_RINGS_PER_QUEUE; i++) {
- vmxnet3_put_ring_to_file(f, &r->rx_ring[i]);
+static const VMStateDescription vmstate_vmxnet3_txq_descr = {
+ .name = "vmxnet3-txq-descr",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
+ Vmxnet3Ring),
+ VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring,
+ Vmxnet3Ring),
+ VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr),
+ VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr),
+ VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_tx_stats,
+ struct UPT1_TxStats),
+ VMSTATE_END_OF_LIST()
}
+};
- vmxnet3_put_ring_to_file(f, &r->comp_ring);
- qemu_put_byte(f, r->intr_idx);
- qemu_put_be64(f, r->rx_stats_pa);
- vmxnet3_put_rx_stats_to_file(f, &r->rxq_stats);
+static const VMStateDescription vmstate_vmxnet3_rx_stats = {
+ .name = "vmxnet3-rx-stats",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(LROPktsRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(LROBytesRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(ucastPktsRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(ucastBytesRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(mcastPktsRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(mcastBytesRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(bcastPktsRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(bcastBytesRxOK, struct UPT1_RxStats),
+ VMSTATE_UINT64(pktsRxOutOfBuf, struct UPT1_RxStats),
+ VMSTATE_UINT64(pktsRxError, struct UPT1_RxStats),
+ VMSTATE_END_OF_LIST()
+ }
+};
- return 0;
-}
+static const VMStateDescription vmstate_vmxnet3_rxq_descr = {
+ .name = "vmxnet3-rxq-descr",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT_ARRAY(rx_ring, Vmxnet3RxqDescr,
+ VMXNET3_RX_RINGS_PER_QUEUE, 0,
+ vmstate_vmxnet3_ring, Vmxnet3Ring),
+ VMSTATE_STRUCT(comp_ring, Vmxnet3RxqDescr, 0, vmstate_vmxnet3_ring,
+ Vmxnet3Ring),
+ VMSTATE_UINT8(intr_idx, Vmxnet3RxqDescr),
+ VMSTATE_UINT64(rx_stats_pa, Vmxnet3RxqDescr),
+ VMSTATE_STRUCT(rxq_stats, Vmxnet3RxqDescr, 0, vmstate_vmxnet3_rx_stats,
+ struct UPT1_RxStats),
+ VMSTATE_END_OF_LIST()
+ }
+};
static int vmxnet3_post_load(void *opaque, int version_id)
{
@@ -2577,40 +2509,15 @@ static int vmxnet3_post_load(void *opaque, int version_id)
return 0;
}
-static const VMStateInfo rxq_descr_info = {
- .name = "rxq_descr",
- .get = vmxnet3_get_rxq_descr,
- .put = vmxnet3_put_rxq_descr
-};
-
-static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size,
- VMStateField *field)
-{
- Vmxnet3IntState *r = pv;
-
- r->is_masked = qemu_get_byte(f);
- r->is_pending = qemu_get_byte(f);
- r->is_asserted = qemu_get_byte(f);
-
- return 0;
-}
-
-static int vmxnet3_put_int_state(QEMUFile *f, void *pv, size_t size,
- VMStateField *field, QJSON *vmdesc)
-{
- Vmxnet3IntState *r = pv;
-
- qemu_put_byte(f, r->is_masked);
- qemu_put_byte(f, r->is_pending);
- qemu_put_byte(f, r->is_asserted);
-
- return 0;
-}
-
-static const VMStateInfo int_state_info = {
- .name = "int_state",
- .get = vmxnet3_get_int_state,
- .put = vmxnet3_put_int_state
+static const VMStateDescription vmstate_vmxnet3_int_state = {
+ .name = "vmxnet3-int-state",
+ .version_id = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_BOOL(is_masked, Vmxnet3IntState),
+ VMSTATE_BOOL(is_pending, Vmxnet3IntState),
+ VMSTATE_BOOL(is_asserted, Vmxnet3IntState),
+ VMSTATE_END_OF_LIST()
+ }
};
static bool vmxnet3_vmstate_need_pcie_device(void *opaque)
@@ -2667,14 +2574,15 @@ static const VMStateDescription vmstate_vmxnet3 = {
VMSTATE_UINT64(drv_shmem, VMXNET3State),
VMSTATE_UINT64(temp_shared_guest_driver_memory, VMXNET3State),
- VMSTATE_ARRAY(txq_descr, VMXNET3State,
- VMXNET3_DEVICE_MAX_TX_QUEUES, 0, txq_descr_info,
+ VMSTATE_STRUCT_ARRAY(txq_descr, VMXNET3State,
+ VMXNET3_DEVICE_MAX_TX_QUEUES, 0, vmstate_vmxnet3_txq_descr,
Vmxnet3TxqDescr),
- VMSTATE_ARRAY(rxq_descr, VMXNET3State,
- VMXNET3_DEVICE_MAX_RX_QUEUES, 0, rxq_descr_info,
+ VMSTATE_STRUCT_ARRAY(rxq_descr, VMXNET3State,
+ VMXNET3_DEVICE_MAX_RX_QUEUES, 0, vmstate_vmxnet3_rxq_descr,
Vmxnet3RxqDescr),
- VMSTATE_ARRAY(interrupt_states, VMXNET3State, VMXNET3_MAX_INTRS,
- 0, int_state_info, Vmxnet3IntState),
+ VMSTATE_STRUCT_ARRAY(interrupt_states, VMXNET3State,
+ VMXNET3_MAX_INTRS, 0, vmstate_vmxnet3_int_state,
+ Vmxnet3IntState),
VMSTATE_END_OF_LIST()
},
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 14/19] net/colo: fix memory double free error
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (12 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 13/19] vmxnet3: VMStatify rx/tx q_descr and int_state Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 15/19] filter-rewriter: skip net_checksum_calculate() while offset = 0 Jason Wang
` (5 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: zhanghailiang, Jason Wang
From: zhanghailiang <zhang.zhanghailiang@huawei.com>
The 'primary_list' and 'secondary_list' members of struct Connection
is not allocated through dynamically g_queue_new(), but we free it by using
g_queue_free(), which will lead to a double-free bug.
Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/colo.c b/net/colo.c
index 6a6eacd..8cc166b 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -147,9 +147,9 @@ void connection_destroy(void *opaque)
Connection *conn = opaque;
g_queue_foreach(&conn->primary_list, packet_destroy, NULL);
- g_queue_free(&conn->primary_list);
+ g_queue_clear(&conn->primary_list);
g_queue_foreach(&conn->secondary_list, packet_destroy, NULL);
- g_queue_free(&conn->secondary_list);
+ g_queue_clear(&conn->secondary_list);
g_slice_free(Connection, conn);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 15/19] filter-rewriter: skip net_checksum_calculate() while offset = 0
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (13 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 14/19] net/colo: fix memory double free error Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 16/19] COLO-compare: Rename compare function and remove duplicate codes Jason Wang
` (4 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: zhanghailiang, Jason Wang
From: zhanghailiang <zhang.zhanghailiang@huawei.com>
While the offset of packets's sequence for primary side and
secondary side is zero, it is unnecessary to call net_checksum_calculate()
to recalculate the checksume value of packets.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/filter-rewriter.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index c4ab91c..afa06e8 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -93,10 +93,12 @@ static int handle_primary_tcp_pkt(NetFilterState *nf,
conn->offset -= (ntohl(tcp_pkt->th_ack) - 1);
conn->syn_flag = 0;
}
- /* handle packets to the secondary from the primary */
- tcp_pkt->th_ack = htonl(ntohl(tcp_pkt->th_ack) + conn->offset);
+ if (conn->offset) {
+ /* handle packets to the secondary from the primary */
+ tcp_pkt->th_ack = htonl(ntohl(tcp_pkt->th_ack) + conn->offset);
- net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ }
}
return 0;
@@ -129,10 +131,13 @@ static int handle_secondary_tcp_pkt(NetFilterState *nf,
}
if ((tcp_pkt->th_flags & (TH_ACK | TH_SYN)) == TH_ACK) {
- /* handle packets to the primary from the secondary*/
- tcp_pkt->th_seq = htonl(ntohl(tcp_pkt->th_seq) - conn->offset);
+ /* Only need to adjust seq while offset is Non-zero */
+ if (conn->offset) {
+ /* handle packets to the primary from the secondary*/
+ tcp_pkt->th_seq = htonl(ntohl(tcp_pkt->th_seq) - conn->offset);
- net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ net_checksum_calculate((uint8_t *)pkt->data, pkt->size);
+ }
}
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 16/19] COLO-compare: Rename compare function and remove duplicate codes
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (14 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 15/19] filter-rewriter: skip net_checksum_calculate() while offset = 0 Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 17/19] COLO-compare: Optimize compare_common and compare_tcp Jason Wang
` (3 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Zhang Chen, Jason Wang
From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Rename colo_packet_compare() to colo_packet_compare_common() that
make tcp_compare udp_compare icmp_compare reuse this function.
Remove minimum packet size check in icmp_compare, because we have
check this in parse_packet_early().
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo-compare.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 300f017..602a758 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -180,7 +180,7 @@ static int packet_enqueue(CompareState *s, int mode)
* return: 0 means packet same
* > 0 || < 0 means packet different
*/
-static int colo_packet_compare(Packet *ppkt, Packet *spkt)
+static int colo_packet_compare_common(Packet *ppkt, Packet *spkt)
{
trace_colo_compare_ip_info(ppkt->size, inet_ntoa(ppkt->ip->ip_src),
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
@@ -190,6 +190,7 @@ static int colo_packet_compare(Packet *ppkt, Packet *spkt)
if (ppkt->size == spkt->size) {
return memcmp(ppkt->data, spkt->data, spkt->size);
} else {
+ trace_colo_compare_main("Net packet size are not the same");
return -1;
}
}
@@ -205,6 +206,7 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
int res;
trace_colo_compare_main("compare tcp");
+
if (ppkt->size != spkt->size) {
if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
trace_colo_compare_main("pkt size not same");
@@ -263,7 +265,8 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
int ret;
trace_colo_compare_main("compare udp");
- ret = colo_packet_compare(ppkt, spkt);
+
+ ret = colo_packet_compare_common(ppkt, spkt);
if (ret) {
trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
@@ -281,16 +284,9 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
*/
static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
{
- int network_length;
-
trace_colo_compare_main("compare icmp");
- network_length = ppkt->ip->ip_hl * 4;
- if (ppkt->size != spkt->size ||
- ppkt->size < network_length + ETH_HLEN) {
- return -1;
- }
- if (colo_packet_compare(ppkt, spkt)) {
+ if (colo_packet_compare_common(ppkt, spkt)) {
trace_colo_compare_icmp_miscompare("primary pkt size",
ppkt->size);
qemu_hexdump((char *)ppkt->data, stderr, "colo-compare",
@@ -316,7 +312,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
inet_ntoa(spkt->ip->ip_src),
inet_ntoa(spkt->ip->ip_dst));
- return colo_packet_compare(ppkt, spkt);
+ return colo_packet_compare_common(ppkt, spkt);
}
static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 17/19] COLO-compare: Optimize compare_common and compare_tcp
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (15 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 16/19] COLO-compare: Rename compare function and remove duplicate codes Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 18/19] COLO-compare: Fix icmp and udp compare different packet always dump bug Jason Wang
` (2 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Zhang Chen, Jason Wang
From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Add offset args for colo_packet_compare_common, optimize
colo_packet_compare_icmp() and colo_packet_compare_udp()
just compare the IP payload. Before compare all tcp packet,
we compare tcp checksum firstly, this function can get
better performance.
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo-compare.c | 50 ++++++++++++++++++++++++++++++++++++--------------
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 602a758..9f5968d 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -180,7 +180,7 @@ static int packet_enqueue(CompareState *s, int mode)
* return: 0 means packet same
* > 0 || < 0 means packet different
*/
-static int colo_packet_compare_common(Packet *ppkt, Packet *spkt)
+static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset)
{
trace_colo_compare_ip_info(ppkt->size, inet_ntoa(ppkt->ip->ip_src),
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
@@ -188,7 +188,8 @@ static int colo_packet_compare_common(Packet *ppkt, Packet *spkt)
inet_ntoa(spkt->ip->ip_dst));
if (ppkt->size == spkt->size) {
- return memcmp(ppkt->data, spkt->data, spkt->size);
+ return memcmp(ppkt->data + offset, spkt->data + offset,
+ spkt->size - offset);
} else {
trace_colo_compare_main("Net packet size are not the same");
return -1;
@@ -207,13 +208,6 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
trace_colo_compare_main("compare tcp");
- if (ppkt->size != spkt->size) {
- if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
- trace_colo_compare_main("pkt size not same");
- }
- return -1;
- }
-
ptcp = (struct tcphdr *)ppkt->transport_header;
stcp = (struct tcphdr *)spkt->transport_header;
@@ -231,8 +225,11 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
spkt->ip->ip_sum = ppkt->ip->ip_sum;
}
- res = memcmp(ppkt->data + ETH_HLEN, spkt->data + ETH_HLEN,
- (spkt->size - ETH_HLEN));
+ if (ptcp->th_sum == stcp->th_sum) {
+ res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
+ } else {
+ res = -1;
+ }
if (res != 0 && trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
trace_colo_compare_pkt_info_src(inet_ntoa(ppkt->ip->ip_src),
@@ -263,10 +260,22 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
{
int ret;
+ int network_header_length = ppkt->ip->ip_hl * 4;
trace_colo_compare_main("compare udp");
- ret = colo_packet_compare_common(ppkt, spkt);
+ /*
+ * Because of ppkt and spkt are both in the same connection,
+ * The ppkt's src ip, dst ip, src port, dst port, ip_proto all are
+ * same with spkt. In addition, IP header's Identification is a random
+ * field, we can handle it in IP fragmentation function later.
+ * COLO just concern the response net packet payload from primary guest
+ * and secondary guest are same or not, So we ignored all IP header include
+ * other field like TOS,TTL,IP Checksum. we only need to compare
+ * the ip payload here.
+ */
+ ret = colo_packet_compare_common(ppkt, spkt,
+ network_header_length + ETH_HLEN);
if (ret) {
trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
@@ -284,9 +293,22 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
*/
static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
{
+ int network_header_length = ppkt->ip->ip_hl * 4;
+
trace_colo_compare_main("compare icmp");
- if (colo_packet_compare_common(ppkt, spkt)) {
+ /*
+ * Because of ppkt and spkt are both in the same connection,
+ * The ppkt's src ip, dst ip, src port, dst port, ip_proto all are
+ * same with spkt. In addition, IP header's Identification is a random
+ * field, we can handle it in IP fragmentation function later.
+ * COLO just concern the response net packet payload from primary guest
+ * and secondary guest are same or not, So we ignored all IP header include
+ * other field like TOS,TTL,IP Checksum. we only need to compare
+ * the ip payload here.
+ */
+ if (colo_packet_compare_common(ppkt, spkt,
+ network_header_length + ETH_HLEN)) {
trace_colo_compare_icmp_miscompare("primary pkt size",
ppkt->size);
qemu_hexdump((char *)ppkt->data, stderr, "colo-compare",
@@ -312,7 +334,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
inet_ntoa(spkt->ip->ip_src),
inet_ntoa(spkt->ip->ip_dst));
- return colo_packet_compare_common(ppkt, spkt);
+ return colo_packet_compare_common(ppkt, spkt, 0);
}
static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 18/19] COLO-compare: Fix icmp and udp compare different packet always dump bug
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (16 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 17/19] COLO-compare: Optimize compare_common and compare_tcp Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 19/19] net/filter-mirror: Follow CODING_STYLE Jason Wang
2017-03-07 7:31 ` [Qemu-devel] [PULL RESEND 00/19] Net patches Peter Maydell
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Zhang Chen, Jason Wang
From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/colo-compare.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 9f5968d..282727b 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -279,9 +279,13 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
if (ret) {
trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
- qemu_hexdump((char *)ppkt->data, stderr, "colo-compare", ppkt->size);
trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
- qemu_hexdump((char *)spkt->data, stderr, "colo-compare", spkt->size);
+ if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
+ qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+ ppkt->size);
+ qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+ spkt->size);
+ }
}
return ret;
@@ -311,12 +315,14 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
network_header_length + ETH_HLEN)) {
trace_colo_compare_icmp_miscompare("primary pkt size",
ppkt->size);
- qemu_hexdump((char *)ppkt->data, stderr, "colo-compare",
- ppkt->size);
trace_colo_compare_icmp_miscompare("Secondary pkt size",
spkt->size);
- qemu_hexdump((char *)spkt->data, stderr, "colo-compare",
- spkt->size);
+ if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
+ qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
+ ppkt->size);
+ qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
+ spkt->size);
+ }
return -1;
} else {
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL RESEND 19/19] net/filter-mirror: Follow CODING_STYLE
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (17 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 18/19] COLO-compare: Fix icmp and udp compare different packet always dump bug Jason Wang
@ 2017-03-06 5:25 ` Jason Wang
2017-03-07 7:31 ` [Qemu-devel] [PULL RESEND 00/19] Net patches Peter Maydell
19 siblings, 0 replies; 31+ messages in thread
From: Jason Wang @ 2017-03-06 5:25 UTC (permalink / raw)
To: peter.maydell, qemu-devel; +Cc: Zhang Chen, Jason Wang
From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/filter-mirror.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index aa0aa98..72fa7c2 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -49,7 +49,7 @@ static int filter_mirror_send(CharBackend *chr_out,
{
int ret = 0;
ssize_t size = 0;
- uint32_t len = 0;
+ uint32_t len = 0;
char *buf;
size = iov_size(iov, iovcnt);
@@ -77,8 +77,9 @@ err:
return ret < 0 ? ret : -EIO;
}
-static void
-redirector_to_filter(NetFilterState *nf, const uint8_t *buf, int len)
+static void redirector_to_filter(NetFilterState *nf,
+ const uint8_t *buf,
+ int len)
{
struct iovec iov = {
.iov_base = (void *)buf,
--
2.7.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL RESEND 00/19] Net patches
2017-03-06 5:25 [Qemu-devel] [PULL RESEND 00/19] Net patches Jason Wang
` (18 preceding siblings ...)
2017-03-06 5:25 ` [Qemu-devel] [PULL RESEND 19/19] net/filter-mirror: Follow CODING_STYLE Jason Wang
@ 2017-03-07 7:31 ` Peter Maydell
19 siblings, 0 replies; 31+ messages in thread
From: Peter Maydell @ 2017-03-07 7:31 UTC (permalink / raw)
To: Jason Wang; +Cc: QEMU Developers
On 6 March 2017 at 05:25, Jason Wang <jasowang@redhat.com> wrote:
> The following changes since commit 17783ac828adc694d986698d2d7014aedfeb48c6:
>
> Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging (2017-03-04 16:31:14 +0000)
>
> are available in the git repository at:
>
> https://github.com/jasowang/qemu.git tags/net-pull-request
>
> for you to fetch changes up to f0aabd5c4ae11fde704d138e8f06c69e5c902a16:
>
> net/filter-mirror: Follow CODING_STYLE (2017-03-06 11:46:02 +0800)
>
> ----------------------------------------------------------------
> Preview pull does not reach list.
>
> - fix vlan issues for e1000e
> - convert to use vmstate for vmxnet3
> - several fixes for colo
> ----------------------------------------------------------------
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 31+ messages in thread