DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] virtio: Fix vring entry number issue
@ 2014-09-04  6:34 Ouyang Changchun
  2014-10-01 11:48 ` Olivier MATZ
  0 siblings, 1 reply; 3+ messages in thread
From: Ouyang Changchun @ 2014-09-04  6:34 UTC (permalink / raw)
  To: dev

Fix one issue in virtio TX: it needs one more vring entry to hold the virtio header when transmitting packets, it is used later to determine whether to free more entries from used vring.

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
Reviewed-by: Huawei Xie <huawei.xie@intel.com>
Tested-by: Jingguo Fu <jingguox.fu@intel.com>
---
 lib/librte_pmd_virtio/virtio_rxtx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_pmd_virtio/virtio_rxtx.c b/lib/librte_pmd_virtio/virtio_rxtx.c
index 0b10108..b1c189c 100644
--- a/lib/librte_pmd_virtio/virtio_rxtx.c
+++ b/lib/librte_pmd_virtio/virtio_rxtx.c
@@ -699,7 +699,8 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	num = (uint16_t)(likely(nb_used < VIRTIO_MBUF_BURST_SZ) ? nb_used : VIRTIO_MBUF_BURST_SZ);
 
 	while (nb_tx < nb_pkts) {
-		int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt;
+		/* Need one more entry for virtio header. */
+		int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt + 1;
 		int deq_cnt = RTE_MIN(need, (int)num);
 
 		num -= (deq_cnt > 0) ? deq_cnt : 0;
-- 
1.8.4.2

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-dev] [PATCH] virtio: Fix vring entry number issue
  2014-09-04  6:34 [dpdk-dev] [PATCH] virtio: Fix vring entry number issue Ouyang Changchun
@ 2014-10-01 11:48 ` Olivier MATZ
  2014-10-11  8:22   ` Ouyang, Changchun
  0 siblings, 1 reply; 3+ messages in thread
From: Olivier MATZ @ 2014-10-01 11:48 UTC (permalink / raw)
  To: Ouyang Changchun, dev

Hello,

On 09/04/2014 08:34 AM, Ouyang Changchun wrote:
> Fix one issue in virtio TX: it needs one more vring entry to hold
> the virtio header when transmitting packets, it is used later to
> determine whether to free more entries from used vring.
>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> Reviewed-by: Huawei Xie <huawei.xie@intel.com>
> Tested-by: Jingguo Fu <jingguox.fu@intel.com>
> ---
>   lib/librte_pmd_virtio/virtio_rxtx.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_pmd_virtio/virtio_rxtx.c b/lib/librte_pmd_virtio/virtio_rxtx.c
> index 0b10108..b1c189c 100644
> --- a/lib/librte_pmd_virtio/virtio_rxtx.c
> +++ b/lib/librte_pmd_virtio/virtio_rxtx.c
> @@ -699,7 +699,8 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>   	num = (uint16_t)(likely(nb_used < VIRTIO_MBUF_BURST_SZ) ? nb_used : VIRTIO_MBUF_BURST_SZ);
>
>   	while (nb_tx < nb_pkts) {
> -		int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt;
> +		/* Need one more entry for virtio header. */
> +		int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt + 1;
>   		int deq_cnt = RTE_MIN(need, (int)num);
>
>   		num -= (deq_cnt > 0) ? deq_cnt : 0;
>


In the commit log, you talk about vring entries, but to me it seems that
it is about virtio descriptors.

I think it could be useful to explain what was the consequence of this
issue. Is it a performance issue? In my understanding, the problem is
that we won't dequeue mbufs from the "used" vring, resulting in a
possible "blocking" of the queue. Is it correct? Also, I think it would
help the review to explain in which conditions the problem is triggered
and how the fix was tested.

Last comment, I'm wondering if the next test should also be modified:

	if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt) {

Into:

	if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt + 1) {

	(or maybe using the "need" variable)

Do you agree?

Regards,
Olivier

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-dev] [PATCH] virtio: Fix vring entry number issue
  2014-10-01 11:48 ` Olivier MATZ
@ 2014-10-11  8:22   ` Ouyang, Changchun
  0 siblings, 0 replies; 3+ messages in thread
From: Ouyang, Changchun @ 2014-10-11  8:22 UTC (permalink / raw)
  To: Olivier MATZ, dev

Hi Olivier,

I have v2 patch according to your comments.
Please Ack it if you can.

Thanks,
Changchun

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, October 1, 2014 7:49 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] virtio: Fix vring entry number issue
> 
> Hello,
> 
> On 09/04/2014 08:34 AM, Ouyang Changchun wrote:
> > Fix one issue in virtio TX: it needs one more vring entry to hold the
> > virtio header when transmitting packets, it is used later to determine
> > whether to free more entries from used vring.
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > Reviewed-by: Huawei Xie <huawei.xie@intel.com>
> > Tested-by: Jingguo Fu <jingguox.fu@intel.com>
> > ---
> >   lib/librte_pmd_virtio/virtio_rxtx.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_pmd_virtio/virtio_rxtx.c
> > b/lib/librte_pmd_virtio/virtio_rxtx.c
> > index 0b10108..b1c189c 100644
> > --- a/lib/librte_pmd_virtio/virtio_rxtx.c
> > +++ b/lib/librte_pmd_virtio/virtio_rxtx.c
> > @@ -699,7 +699,8 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pkts)
> >   	num = (uint16_t)(likely(nb_used < VIRTIO_MBUF_BURST_SZ) ?
> nb_used :
> > VIRTIO_MBUF_BURST_SZ);
> >
> >   	while (nb_tx < nb_pkts) {
> > -		int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq-
> >vq_free_cnt;
> > +		/* Need one more entry for virtio header. */
> > +		int need = tx_pkts[nb_tx]->pkt.nb_segs - txvq->vq_free_cnt
> + 1;
> >   		int deq_cnt = RTE_MIN(need, (int)num);
> >
> >   		num -= (deq_cnt > 0) ? deq_cnt : 0;
> >
> 
> 
> In the commit log, you talk about vring entries, but to me it seems that it is
> about virtio descriptors.
> 
Agree, but in current virtio pmd, one entry has only one descriptor, so both are ok.
To be more accurate, in v2 patch I adopt your suggestion. :-)

> I think it could be useful to explain what was the consequence of this issue. Is
> it a performance issue? In my understanding, the problem is that we won't

Not only performance issue,  as I state in the v2 patch description, 
In circumstance of only 1 descriptor in free list, the packet with only 1 segment can't be transmitted
Because the transmitting need 2 descriptors(one for packet itself, the other for virtio header), but only 1 freed descriptor available.
And due to the false test condition, no more descriptors will be freed into free list.

> dequeue mbufs from the "used" vring, resulting in a possible "blocking" of
> the queue. Is it correct? Also, I think it would help the review to explain in
> which conditions the problem is triggered and how the fix was tested.
> 
> Last comment, I'm wondering if the next test should also be modified:
> 
> 	if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt) {
> 
> Into:
> 
> 	if (tx_pkts[nb_tx]->nb_segs <= txvq->vq_free_cnt + 1) {
> 
> 	(or maybe using the "need" variable)
> 
> Do you agree?

Agree, change is made in v2 patch.

> 
> Regards,
> Olivier

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2014-10-11  8:14 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-04  6:34 [dpdk-dev] [PATCH] virtio: Fix vring entry number issue Ouyang Changchun
2014-10-01 11:48 ` Olivier MATZ
2014-10-11  8:22   ` Ouyang, Changchun

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).