* [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq
@ 2012-02-15 8:13 zwu.kernel
2012-02-15 8:30 ` Jan Kiszka
0 siblings, 1 reply; 3+ messages in thread
From: zwu.kernel @ 2012-02-15 8:13 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, Zhi Yong Wu, jan.kiszka, stefanha, mst
From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
This patch fixes the slirp crash in current QEMU upstream.
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
slirp/if.c | 37 ++++++++++++++++++++++++++++++-------
slirp/mbuf.c | 3 +--
2 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/slirp/if.c b/slirp/if.c
index 8e0cac2..f7f8577 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -20,8 +20,15 @@ ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead)
static void
ifs_remque(struct mbuf *ifm)
{
- ifm->ifs_prev->ifs_next = ifm->ifs_next;
- ifm->ifs_next->ifs_prev = ifm->ifs_prev;
+ if (ifm->ifs_next->ifs_next == ifm
+ && ifm->ifs_next->ifs_prev == ifm) {
+ ifs_init(ifm->ifs_next);
+ } else {
+ ifm->ifs_prev->ifs_next = ifm->ifs_next;
+ ifm->ifs_next->ifs_prev = ifm->ifs_prev;
+ }
+
+ ifs_init(ifm);
}
void
@@ -154,14 +161,18 @@ 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");
+ DEBUG_CALL("if_start");
- if (slirp->if_queued == 0)
- return; /* Nothing to do */
+ if (slirp->if_queued == 0)
+ return; /* Nothing to do */
+
+ slirp->next_m = &slirp->if_batchq;
again:
+ ifm_next = NULL;
+
/* check if we can really output */
if (!slirp_can_output(slirp->opaque))
return;
@@ -190,6 +201,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 +221,18 @@ if_start(Slirp *slirp)
m_free(ifm);
} else {
/* re-queue */
- insque(ifm, ifqt);
+ if (ifm_next) {
+ /*restore the original state of bachq*/
+ 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++;
}
}
diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index c699c75..f429c0a 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -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:
--
1.7.6
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq
2012-02-15 8:13 [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq zwu.kernel
@ 2012-02-15 8:30 ` Jan Kiszka
2012-02-16 7:34 ` Zhi Yong Wu
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kiszka @ 2012-02-15 8:30 UTC (permalink / raw)
To: zwu.kernel; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst
[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]
On 2012-02-15 09:13, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>
> This patch fixes the slirp crash in current QEMU upstream.
>
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
> slirp/if.c | 37 ++++++++++++++++++++++++++++++-------
> slirp/mbuf.c | 3 +--
> 2 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/slirp/if.c b/slirp/if.c
> index 8e0cac2..f7f8577 100644
> --- a/slirp/if.c
> +++ b/slirp/if.c
> @@ -20,8 +20,15 @@ ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead)
> static void
> ifs_remque(struct mbuf *ifm)
> {
> - ifm->ifs_prev->ifs_next = ifm->ifs_next;
> - ifm->ifs_next->ifs_prev = ifm->ifs_prev;
> + if (ifm->ifs_next->ifs_next == ifm
> + && ifm->ifs_next->ifs_prev == ifm) {
> + ifs_init(ifm->ifs_next);
> + } else {
> + ifm->ifs_prev->ifs_next = ifm->ifs_next;
> + ifm->ifs_next->ifs_prev = ifm->ifs_prev;
> + }
> +
> + ifs_init(ifm);
This looks, well, interesting. Can you explain the logic? We really need
to document the queuing mechanics.
> }
>
> void
> @@ -154,14 +161,18 @@ 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");
> + DEBUG_CALL("if_start");
>
> - if (slirp->if_queued == 0)
> - return; /* Nothing to do */
> + if (slirp->if_queued == 0)
> + return; /* Nothing to do */
Even if the result looks funny, let's not touch lines just for indention
(and braces would be missing anyway).
> +
> + slirp->next_m = &slirp->if_batchq;
Have you understood the difference between the natural order of
if_batchq and next_m? I still wonder what the latter is good for.
>
> again:
> + ifm_next = NULL;
> +
> /* check if we can really output */
> if (!slirp_can_output(slirp->opaque))
> return;
> @@ -190,6 +201,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 +221,18 @@ if_start(Slirp *slirp)
> m_free(ifm);
> } else {
> /* re-queue */
> - insque(ifm, ifqt);
> + if (ifm_next) {
> + /*restore the original state of bachq*/
> + 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;
So is this only about the correct ordering or also about pointer
correctness?
> + } else {
> + insque(ifm, ifqt);
> + }
> +
> requeued++;
> }
> }
> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> index c699c75..f429c0a 100644
> --- a/slirp/mbuf.c
> +++ b/slirp/mbuf.c
> @@ -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:
Thanks for digging into this, but I really think it needs more comments
and potentially even some cleanups.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq
2012-02-15 8:30 ` Jan Kiszka
@ 2012-02-16 7:34 ` Zhi Yong Wu
0 siblings, 0 replies; 3+ messages in thread
From: Zhi Yong Wu @ 2012-02-16 7:34 UTC (permalink / raw)
To: Jan Kiszka; +Cc: aliguori, Zhi Yong Wu, qemu-devel, stefanha, mst
On Wed, Feb 15, 2012 at 4:30 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-02-15 09:13, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> This patch fixes the slirp crash in current QEMU upstream.
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>> slirp/if.c | 37 ++++++++++++++++++++++++++++++-------
>> slirp/mbuf.c | 3 +--
>> 2 files changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/slirp/if.c b/slirp/if.c
>> index 8e0cac2..f7f8577 100644
>> --- a/slirp/if.c
>> +++ b/slirp/if.c
>> @@ -20,8 +20,15 @@ ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead)
>> static void
>> ifs_remque(struct mbuf *ifm)
>> {
>> - ifm->ifs_prev->ifs_next = ifm->ifs_next;
>> - ifm->ifs_next->ifs_prev = ifm->ifs_prev;
>> + if (ifm->ifs_next->ifs_next == ifm
>> + && ifm->ifs_next->ifs_prev == ifm) {
>> + ifs_init(ifm->ifs_next);
>> + } else {
>> + ifm->ifs_prev->ifs_next = ifm->ifs_next;
>> + ifm->ifs_next->ifs_prev = ifm->ifs_prev;
>> + }
>> +
>> + ifs_init(ifm);
>
> This looks, well, interesting. Can you explain the logic? We really need
> to document the queuing mechanics.
ifm->ifs_prev->ifs_next = ifm->ifs_next;
ifm->ifs_next->ifs_prev = ifm->ifs_prev;
+ ifs_init(ifm);
Sorry, actually we only need to add one line of code.
>
>> }
>>
>> void
>> @@ -154,14 +161,18 @@ 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");
>> + DEBUG_CALL("if_start");
>>
>> - if (slirp->if_queued == 0)
>> - return; /* Nothing to do */
>> + if (slirp->if_queued == 0)
>> + return; /* Nothing to do */
>
> Even if the result looks funny, let's not touch lines just for indention
> (and braces would be missing anyway).
OK. undo them to their original state.
>
>> +
>> + slirp->next_m = &slirp->if_batchq;
>
> Have you understood the difference between the natural order of
> if_batchq and next_m? I still wonder what the latter is good for.
Sorry, the line of code should be removed. next_m will point to next
packet which will be handled, if there's multiple session, it will
point to the head packet for next session; if there's one session, it
will point to batchq.
>
>>
>> again:
>> + ifm_next = NULL;
>> +
>> /* check if we can really output */
>> if (!slirp_can_output(slirp->opaque))
>> return;
>> @@ -190,6 +201,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 +221,18 @@ if_start(Slirp *slirp)
>> m_free(ifm);
>> } else {
>> /* re-queue */
>> - insque(ifm, ifqt);
>> + if (ifm_next) {
>> + /*restore the original state of bachq*/
>> + 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;
>
> So is this only about the correct ordering or also about pointer
> correctness?
The former. If ifm need to be re-queued, it will be put to where it
originally is.
>
>> + } else {
>> + insque(ifm, ifqt);
>> + }
>> +
>> requeued++;
>> }
>> }
>> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
>> index c699c75..f429c0a 100644
>> --- a/slirp/mbuf.c
>> +++ b/slirp/mbuf.c
>> @@ -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:
>
> Thanks for digging into this, but I really think it needs more comments
> and potentially even some cleanups.
>
> Jan
>
--
Regards,
Zhi Yong Wu
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-02-16 7:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-15 8:13 [Qemu-devel] [PATCH 2/2] slirp: fix packet requeue issue in batchq zwu.kernel
2012-02-15 8:30 ` Jan Kiszka
2012-02-16 7:34 ` Zhi Yong Wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).