slirp: fix packet requeue issue in batchq
PATCH v2 1/1 slirp: fix packet requeue issue in batchq
From: Zhi Yong Wu <[email protected]>
--- slirp/if.c | 19 +++++++++++++++++-- slirp/mbuf.c | 3 +-- 2 files changed, 18 insertions(+), 4 deletions(-)
slirp/if.c +17 -2
@@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; +ifs_init (ifm); }void
@@ -154,7 +155,7 @@ if_start(Slirp *slirp) {uint64_t now =qemu_get_clock_ns (rt_clock);int requeued =0 ; -struct mbuf *ifm, *ifqt; +struct mbuf *ifm, *ifqt, *ifm_next;DEBUG_CALL ("if_start" );
@@ -162,6 +163,8 @@ if_start(Slirp *slirp)return ;/* Nothing to do */ again: + ifm_next =NULL ; +/* check if we can really output */ if (!slirp_can_output (slirp->opaque))return ;
@@ -190,6 +193,7 @@ if_start(Slirp *slirp)/* If there are more packets for this session, re-queue them */ if (ifm->ifs_next !=/* ifm->ifs_prev != */ ifm) {insque (ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next;ifs_remque (ifm); }
@@ -209,7 +213,18 @@ if_start(Slirp *slirp)m_free (ifm); }else {/* re-queue */ -insque (ifm, ifqt); +if (ifm_next) { +/*restore the original state of batchq*/ +remque (ifm_next); +insque (ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + }else { +insque (ifm, ifqt); + } + requeued++; } }
slirp/mbuf.c +1 -2
@@ -68,8 +68,7 @@ m_get(Slirp *slirp) m->m_size =SLIRP_MSIZE - offsetof(struct mbuf , m_dat); m->m_data = m->m_dat; m->m_len =0 ; - m->m_nextpkt =NULL ; - m->m_prevpkt =NULL ; +ifs_init (m); m->arp_requested = false; m->expiration_date = (uint64_t )-1 ; end_error:
Signature · 1 line
1.7.6
Quoted 2 lines · On 2012-02-16 09:07, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
Please summarize in a bit more details what was broken.
Quoted 69 lines
Signed-off-by Zhi Yong Wu <[email protected]>--- slirp/if.c | 19 +++++++++++++++++-- slirp/mbuf.c | 3 +-- 2 files changed, 18 insertions(+), 4 deletions(-)slirp/if.c +17
-2@@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; +ifs_init (ifm); }void @@ -154,7 +155,7 @@ if_start(Slirp *slirp) {uint64_t now =qemu_get_clock_ns (rt_clock);int requeued =0 ; -struct mbuf *ifm, *ifqt; +struct mbuf *ifm, *ifqt, *ifm_next;DEBUG_CALL ("if_start" );@@ -162,6 +163,8 @@ if_start(Slirp *slirp)return ;/* Nothing to do */ again: + ifm_next =NULL ; +/* check if we can really output */ if (!slirp_can_output (slirp->opaque))return ;@@ -190,6 +193,7 @@ if_start(Slirp *slirp)/* If there are more packets for this session, re-queue them */ if (ifm->ifs_next !=/* ifm->ifs_prev != */ ifm) {insque (ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next;ifs_remque (ifm); }@@ -209,7 +213,18 @@ if_start(Slirp *slirp)m_free (ifm); }else {/* re-queue */ -insque (ifm, ifqt); +if (ifm_next) { +/*restore the original state of batchq*/ +remque (ifm_next); +insque (ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + }else { +insque (ifm, ifqt); + } + requeued++; } }slirp/mbuf.c +0
-0
slirp/mbuf.c m_get(Slirp *slirp) · line 74
Quoted 9 lines
slirp/mbuf.c +1
-2@@ -68,8 +68,7 @@ m_get(Slirp *slirp) m->m_size =SLIRP_MSIZE - offsetof(struct mbuf , m_dat); m->m_data = m->m_dat; m->m_len =0 ; - m->m_nextpkt =NULL ; - m->m_prevpkt =NULL ; +ifs_init (m); m->arp_requested = false; m->expiration_date = (uint64_t )-1 ; end_error:
Wondering now: Is this hunk required or a cleanup?
Jan
Quoted 5 lines · Jan Kiszka
Quoted 2 lines · On 2012-02-16 09:07, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
Please summarize in a bit more details what was broken.
Should those bits be put in the message part of this part? or here?
Quoted 73 lines
Quoted 73 lines
Signed-off-by Zhi Yong Wu <[email protected]>---
slirp/if.c | 19 +++++++++++++++++--
slirp/mbuf.c | 3 +--2 files changed, 18 insertions(+), 4 deletions(-)slirp/if.c +1
-0@@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; + ifs_init(ifm); }void
slirp/if.c +1
-1@@ -154,7 +155,7 @@ if_start(Slirp *slirp) { uint64_t now = qemu_get_clock_ns(rt_clock); int requeued = 0; - struct mbuf *ifm, *ifqt; + struct mbuf *ifm, *ifqt, *ifm_next; DEBUG_CALL("if_start");@@ -162,6 +163,8 @@ if_start(Slirp *slirp)return; /* Nothing to do */
again:
+ ifm_next = NULL;
+
/* check if we can really output */
if (!slirp_can_output(slirp->opaque))
return;slirp/if.c +13
-1@@ -190,6 +193,7 @@ if_start(Slirp *slirp) /* If there are more packets for this session, re-queue them */ if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) { insque(ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next; ifs_remque(ifm); }@@ -209,7 +213,18 @@ if_start(Slirp *slirp) m_free(ifm); } else { /* re-queue */ - insque(ifm, ifqt); + if (ifm_next) { + /*restore the original state of batchq*/ + remque(ifm_next); + insque(ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + } else { + insque(ifm, ifqt); + } + requeued++; } }slirp/mbuf.c +0
-0@@ -68,8 +68,7 @@ m_get(Slirp *slirp) m->m_size = SLIRP_MSIZE - offsetof(struct mbuf, m_dat); m->m_data = m->m_dat; m->m_len = 0;
Quoted 8 lines
Quoted 6 lines
slirp/mbuf.c +1
-2- m->m_nextpkt = NULL; - m->m_prevpkt = NULL; + ifs_init(m); m->arp_requested = false; m->expiration_date = (uint64_t)-1;end_error:
Wondering now: Is this hunk required or a cleanup?
The former. I think that the pointer of one raw mbuf which are used to link the packets for the same session should default to itself, not NULL.
Quoted 3 lines
Jan
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 7 lines · Zhi Yong Wu
Quoted 5 lines · Jan Kiszka
Quoted 2 lines · On 2012-02-16 09:07, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
Please summarize in a bit more details what was broken.
Should those bits be put in the message part of this part? or here?
s/this part/this patch/
Quoted 96 lines
Quoted 74 lines
Quoted 74 lines
Signed-off-by Zhi Yong Wu <[email protected]>---
slirp/if.c | 19 +++++++++++++++++--
slirp/mbuf.c | 3 +--2 files changed, 18 insertions(+), 4 deletions(-)slirp/if.c +1
-0@@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; + ifs_init(ifm); }void
slirp/if.c +1
-1@@ -154,7 +155,7 @@ if_start(Slirp *slirp) { uint64_t now = qemu_get_clock_ns(rt_clock); int requeued = 0; - struct mbuf *ifm, *ifqt; + struct mbuf *ifm, *ifqt, *ifm_next; DEBUG_CALL("if_start");@@ -162,6 +163,8 @@ if_start(Slirp *slirp)return; /* Nothing to do */
again:
+ ifm_next = NULL;
+
/* check if we can really output */
if (!slirp_can_output(slirp->opaque))
return;slirp/if.c +13
-1@@ -190,6 +193,7 @@ if_start(Slirp *slirp) /* If there are more packets for this session, re-queue them */ if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) { insque(ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next; ifs_remque(ifm); }@@ -209,7 +213,18 @@ if_start(Slirp *slirp) m_free(ifm); } else { /* re-queue */ - insque(ifm, ifqt); + if (ifm_next) { + /*restore the original state of batchq*/ + remque(ifm_next); + insque(ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + } else { + insque(ifm, ifqt); + } + requeued++; } }slirp/mbuf.c +0
-0@@ -68,8 +68,7 @@ m_get(Slirp *slirp) m->m_size = SLIRP_MSIZE - offsetof(struct mbuf, m_dat); m->m_data = m->m_dat; m->m_len = 0;Quoted 8 lines
Quoted 6 lines
slirp/mbuf.c +1
-2- m->m_nextpkt = NULL; - m->m_prevpkt = NULL; + ifs_init(m); m->arp_requested = false; m->expiration_date = (uint64_t)-1;end_error:
Wondering now: Is this hunk required or a cleanup?
The former. I think that the pointer of one raw mbuf which are used to link the packets for the same session should default to itself, not NULL.
Quoted 3 lines
Jan
--
Regards,Zhi Yong Wu
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 7 lines · Zhi Yong Wu
Quoted 5 lines · Jan Kiszka
Quoted 2 lines · On 2012-02-16 09:07, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
Please summarize in a bit more details what was broken.
Should those bits be put in the message part of this part? or here?
Here, as a commit log.
Quoted 82 lines
Quoted 74 lines
Quoted 74 lines
Signed-off-by Zhi Yong Wu <[email protected]>--- slirp/if.c | 19 +++++++++++++++++-- slirp/mbuf.c | 3 +-- 2 files changed, 18 insertions(+), 4 deletions(-)slirp/if.c +17
-2@@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; +ifs_init (ifm); }void @@ -154,7 +155,7 @@ if_start(Slirp *slirp) {uint64_t now =qemu_get_clock_ns (rt_clock);int requeued =0 ; -struct mbuf *ifm, *ifqt; +struct mbuf *ifm, *ifqt, *ifm_next;DEBUG_CALL ("if_start" );@@ -162,6 +163,8 @@ if_start(Slirp *slirp)return ;/* Nothing to do */ again: + ifm_next =NULL ; +/* check if we can really output */ if (!slirp_can_output (slirp->opaque))return ;@@ -190,6 +193,7 @@ if_start(Slirp *slirp)/* If there are more packets for this session, re-queue them */ if (ifm->ifs_next !=/* ifm->ifs_prev != */ ifm) {insque (ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next;ifs_remque (ifm); }@@ -209,7 +213,18 @@ if_start(Slirp *slirp)m_free (ifm); }else {/* re-queue */ -insque (ifm, ifqt); +if (ifm_next) { +/*restore the original state of batchq*/ +remque (ifm_next); +insque (ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + }else { +insque (ifm, ifqt); + } + requeued++; } }slirp/mbuf.c +0
-0@@ -68,8 +68,7 @@ m_get(Slirp *slirp) m->m_size =SLIRP_MSIZE - offsetof(struct mbuf , m_dat); m->m_data = m->m_dat; m->m_len =0 ;Quoted 8 lines
slirp/mbuf.c m_get(Slirp *slirp) · line 74
Quoted 6 lines
slirp/mbuf.c +1
-2- m->m_nextpkt =NULL ; - m->m_prevpkt =NULL ; +ifs_init (m); m->arp_requested = false; m->expiration_date = (uint64_t )-1 ; end_error:Wondering now: Is this hunk required or a cleanup?
Quoted 3 lines
The former. I think that the pointer of one raw mbuf which are used to link the packets for the same session should default to itself, not NULL.
OK. Out of curiosity, is that an older issue or just related to the requeuing we now practice?
Jan
Quoted 10 lines · Jan Kiszka
Quoted 7 lines · Zhi Yong Wu
Quoted 5 lines · Jan Kiszka
Quoted 2 lines · On 2012-02-16 09:07, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
Please summarize in a bit more details what was broken.
Should those bits be put in the message part of this part? or here?
Here, as a commit log.
In slirp, The condition ifm->ifs_next == ifm is used to determine if one session only has one packet to handled. But sometime its pointers is default to NULL. Moreover, if one session has multiple packet to be handled, when one packet need to re-queued, the origianl code only simply inserted back to batchq, this will cause that one socket will correspone to multiple doubly linked list of mbuf, but ifs_next[prev] pointer of this re-queued mbuf still keeps its original value, this is wrong.
batchq<-ifq_next[prev]--->mbuf1[socket1]
<-ifq_next[prev]--->-->mbuf4[socket2]<--->...
^| (ifs_next[prev])
mbuf2[socket1]
^| (ifs_next[prev]))
mbuf3[socket1]
^| (ifs_next[prev]))After re-queue
batchq<--ifs_next[prev])-->mbuf1[socket1] <--ifs_next[prev])->mbuf2[socket1]<--ifs_next[prev])->mbuf4[socket2]<--->...
^| (ifs_next[prev])
^| (ifs_next[prev])
mbuf3[socket1]
^| (ifs_next[prev])This is wrong.
This patch fixes this, when one packet is re-queued, the packets for the same session will be put to its original location
Quoted 83 lines
Quoted 83 lines
Quoted 83 lines
Quoted 80 lines
Signed-off-by Zhi Yong Wu <[email protected]>---
slirp/if.c | 19 +++++++++++++++++--
slirp/mbuf.c | 3 +--2 files changed, 18 insertions(+), 4 deletions(-)slirp/if.c +1
-0@@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; + ifs_init(ifm); }void
slirp/if.c +1
-1@@ -154,7 +155,7 @@ if_start(Slirp *slirp) { uint64_t now = qemu_get_clock_ns(rt_clock); int requeued = 0; - struct mbuf *ifm, *ifqt; + struct mbuf *ifm, *ifqt, *ifm_next; DEBUG_CALL("if_start");@@ -162,6 +163,8 @@ if_start(Slirp *slirp)return; /* Nothing to do */
again:
+ ifm_next = NULL;
+
/* check if we can really output */
if (!slirp_can_output(slirp->opaque))
return;slirp/if.c +13
-1@@ -190,6 +193,7 @@ if_start(Slirp *slirp) /* If there are more packets for this session, re-queue them */ if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) { insque(ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next; ifs_remque(ifm); }@@ -209,7 +213,18 @@ if_start(Slirp *slirp) m_free(ifm); } else { /* re-queue */ - insque(ifm, ifqt); + if (ifm_next) { + /*restore the original state of batchq*/ + remque(ifm_next); + insque(ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + } else { + insque(ifm, ifqt); + } + requeued++; } }slirp/mbuf.c +1
-2@@ -68,8 +68,7 @@ m_get(Slirp *slirp) m->m_size = SLIRP_MSIZE - offsetof(struct mbuf, m_dat); m->m_data = m->m_dat; m->m_len = 0; - m->m_nextpkt = NULL; - m->m_prevpkt = NULL; + ifs_init(m); m->arp_requested = false; m->expiration_date = (uint64_t)-1;Quoted 1 line
end_error:
Wondering now: Is this hunk required or a cleanup?
Quoted 6 lines
Quoted 3 lines
The former. I think that the pointer of one raw mbuf which are used to link the packets for the same session should default to itself, not NULL.
OK. Out of curiosity, is that an older issue or just related to the requeuing we now practice?
I don't know if it is an older issue, but i make sure that it relative the requeuing stuff.
For example, you can see some stuff in ifs_remque().
ifm->ifs_prev->ifs_next = ifm->ifs_next;
ifm->ifs_next->ifs_prev = ifm->ifs_prev;The pointer of one pointer easily casues segment error if this pointer is NULL. This is only one example.
Quoted 3 lines
Jan
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 30 lines · Zhi Yong Wu
Quoted 10 lines · Jan Kiszka
Quoted 7 lines · Zhi Yong Wu
Quoted 5 lines · Jan Kiszka
Quoted 2 lines · On 2012-02-16 09:07, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
Please summarize in a bit more details what was broken.
Should those bits be put in the message part of this part? or here?
Here, as a commit log.
In slirp, The condition
ifm->ifs_next== ifm is used to determine if one session only has one packet to handled. But sometime its pointers is default to NULL. Moreover, if one session has multiple packet to be handled, when one packet need to re-queued, the origianl code only simply inserted back to batchq, this will cause that one socket will correspone to multiple doubly linked list of mbuf, but ifs_next[prev] pointer of this re-queued mbuf still keeps its original value, this is wrong.batchq<-ifq_next[prev]--->mbuf1[socket1]
<-ifq_next[prev]--->-->mbuf4[socket2]<--->...
^| (ifs_next[prev])
mbuf2[socket1]
^| (ifs_next[prev]))
mbuf3[socket1]
^| (ifs_next[prev]))After re-queue
Quoted 8 lines
batchq<--ifs_next[prev])-->mbuf1[socket1] <--ifs_next[prev])->mbuf2[socket1]<--ifs_next[prev])->mbuf4[socket2]<--->... ^| (ifs_next[prev]) ^| (ifs_next[prev])
mbuf3[socket1]
^| (ifs_next[prev])
Sorry, there is wrong with this above figure. It seems to be messy after i sent out this mail.
batchq<--ifq_next[prev])-->mbuf1[socket1]
<--ifq_next[prev])->mbuf2[socket1]<--ifq_next[prev])->mbuf4[socket2]<--->...
^| (ifs_next[prev])
^| (ifs_next[prev])
mbuf3[socket1]
^| (ifs_next[prev])Quoted 115 lines
This is wrong.
This patch fixes this, when one packet is re-queued, the packets for the same session will be put to its original location
Quoted 83 lines
Quoted 83 lines
Quoted 83 lines
Quoted 80 lines
Signed-off-by Zhi Yong Wu <[email protected]>---
slirp/if.c | 19 +++++++++++++++++--
slirp/mbuf.c | 3 +--2 files changed, 18 insertions(+), 4 deletions(-)slirp/if.c +1
-0@@ -22,6 +22,7 @@ ifs_remque(struct mbuf *ifm) { ifm->ifs_prev->ifs_next = ifm->ifs_next; ifm->ifs_next->ifs_prev = ifm->ifs_prev; + ifs_init(ifm); }void
slirp/if.c +1
-1@@ -154,7 +155,7 @@ if_start(Slirp *slirp) { uint64_t now = qemu_get_clock_ns(rt_clock); int requeued = 0; - struct mbuf *ifm, *ifqt; + struct mbuf *ifm, *ifqt, *ifm_next; DEBUG_CALL("if_start");@@ -162,6 +163,8 @@ if_start(Slirp *slirp)return; /* Nothing to do */
again:
+ ifm_next = NULL;
+
/* check if we can really output */
if (!slirp_can_output(slirp->opaque))
return;slirp/if.c +13
-1@@ -190,6 +193,7 @@ if_start(Slirp *slirp) /* If there are more packets for this session, re-queue them */ if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) { insque(ifm->ifs_next, ifqt); + ifm_next = ifm->ifs_next; ifs_remque(ifm); }@@ -209,7 +213,18 @@ if_start(Slirp *slirp) m_free(ifm); } else { /* re-queue */ - insque(ifm, ifqt); + if (ifm_next) { + /*restore the original state of batchq*/ + remque(ifm_next); + insque(ifm, ifqt); + ifm_next->ifs_prev->ifs_next = ifm; + ifm->ifs_prev = ifm_next->ifs_prev; + ifm->ifs_next = ifm_next; + ifm_next->ifs_prev = ifm; + } else { + insque(ifm, ifqt); + } + requeued++; } }slirp/mbuf.c +1
-2@@ -68,8 +68,7 @@ m_get(Slirp *slirp) m->m_size = SLIRP_MSIZE - offsetof(struct mbuf, m_dat); m->m_data = m->m_dat; m->m_len = 0; - m->m_nextpkt = NULL; - m->m_prevpkt = NULL; + ifs_init(m); m->arp_requested = false; m->expiration_date = (uint64_t)-1;Quoted 1 line
end_error:
Wondering now: Is this hunk required or a cleanup?
Quoted 6 lines
Quoted 3 lines
The former. I think that the pointer of one raw mbuf which are used to link the packets for the same session should default to itself, not NULL.
OK. Out of curiosity, is that an older issue or just related to the requeuing we now practice?
I don't know if it is an older issue, but i make sure that it relative the requeuing stuff.
For example, you can see some stuff in
ifs_remque().ifm->ifs_prev->ifs_next=ifm->ifs_next;ifm->ifs_next->ifs_prev=ifm->ifs_prev;The pointer of one pointer easily casues segment error if this pointer is NULL. This is only one example.
Quoted 3 lines
Jan
--
Regards,Zhi Yong Wu
Signature · 3 lines
Regards, Zhi Yong Wu