[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On Wed, Mar 9, 2016 at 4:37 PM, Bruce Richardson wrote: > On Fri, Mar 04, 2016 at 08:25:07AM -0500, Kyle Larose wrote: >> On Fri, Mar 4, 2016 at 3:11 AM, Tom Kiely wrote: >> > Sure. >> >Tom >> > >> > >> > On 03/04/2016 06:16 AM, Xie, Huawei wrote: >> >> >> >> On 2/23/2016 12:23 AM, Tom Kiely wrote: >> >>> >> >>> Hi, >> >>> Sorry I missed the last few messages until now. I'm happy with >> >>> just removing the "if". Kyle, when you say you fixed it, do you mean >> >>> that you will push the patch or have already done so ? >> >>> Thanks, >> >>> Tom >> >> >> >> Could you please send the patch? >> >> >> > >> >> I should have replied to this earlier. I submitted a patch last week: >> http://dpdk.org/dev/patchwork/patch/10904/ > > Thanks, Kyle. Unfortunately the patch you submitted is missing your signoff. > Can you perhaps resubmit it as a V2 with the necessary sign-off as described > in the contributors guide: > http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body > Hey Bruce, Thanks. I signed off, and resubmitted the patch. Hopefully I didn't make any other amateur mistakes this time! > Huawei or Tom, could one of you guys perhaps review and ack the patch once > it's > submitted with a signoff? > > Thanks, > /Bruce
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On Fri, Mar 04, 2016 at 08:25:07AM -0500, Kyle Larose wrote: > On Fri, Mar 4, 2016 at 3:11 AM, Tom Kiely wrote: > > Sure. > >Tom > > > > > > On 03/04/2016 06:16 AM, Xie, Huawei wrote: > >> > >> On 2/23/2016 12:23 AM, Tom Kiely wrote: > >>> > >>> Hi, > >>> Sorry I missed the last few messages until now. I'm happy with > >>> just removing the "if". Kyle, when you say you fixed it, do you mean > >>> that you will push the patch or have already done so ? > >>> Thanks, > >>> Tom > >> > >> Could you please send the patch? > >> > > > > I should have replied to this earlier. I submitted a patch last week: > http://dpdk.org/dev/patchwork/patch/10904/ Thanks, Kyle. Unfortunately the patch you submitted is missing your signoff. Can you perhaps resubmit it as a V2 with the necessary sign-off as described in the contributors guide: http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body Huawei or Tom, could one of you guys perhaps review and ack the patch once it's submitted with a signoff? Thanks, /Bruce
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On Fri, Mar 4, 2016 at 3:11 AM, Tom Kiely wrote: > Sure. >Tom > > > On 03/04/2016 06:16 AM, Xie, Huawei wrote: >> >> On 2/23/2016 12:23 AM, Tom Kiely wrote: >>> >>> Hi, >>> Sorry I missed the last few messages until now. I'm happy with >>> just removing the "if". Kyle, when you say you fixed it, do you mean >>> that you will push the patch or have already done so ? >>> Thanks, >>> Tom >> >> Could you please send the patch? >> > I should have replied to this earlier. I submitted a patch last week: http://dpdk.org/dev/patchwork/patch/10904/
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
Sure. Tom On 03/04/2016 06:16 AM, Xie, Huawei wrote: > On 2/23/2016 12:23 AM, Tom Kiely wrote: >> Hi, >> Sorry I missed the last few messages until now. I'm happy with >> just removing the "if". Kyle, when you say you fixed it, do you mean >> that you will push the patch or have already done so ? >> Thanks, >> Tom > Could you please send the patch? >
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On 2/23/2016 12:23 AM, Tom Kiely wrote: > Hi, > Sorry I missed the last few messages until now. I'm happy with > just removing the "if". Kyle, when you say you fixed it, do you mean > that you will push the patch or have already done so ? >Thanks, >Tom Could you please send the patch?
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
Virtio has an mbuf descriptor ring containing mbufs to be used for receiving traffic. When the host queues traffic to be sent to the guest, it consumes these descriptors. If none exist, it discards the packet. The virtio pmd allocates mbufs to the descriptor ring every time it succesfully receives a packet. However, it never does it if it does not receive a valid packet. If the descriptor ring is exhausted, and the mbuf mempool does not have any mbufs free (which can happen for various reasons, such as queueing along the processing pipeline), then the receive call will not allocate any mbufs to the descriptor ring, and when it finishes, the descriptor ring will be empty. The ring being empty means that we will never receive a packet again, which means we will never allocate mbufs to the ring: we are stuck. Ultimately, the problem arises because there is a dependency between receiving packets and making the descriptor ring not be empty, and a dependency between the descriptor ring not being empty, and receicing packets. To fix the problem, this pakes makes virtio always try to allocate mbufs to the descriptor ring, if necessary, when polling for packets. Do this by removing the early exit if no packets were received. Since the packet loop later will do nothing if there are no packets, this is fine. --- drivers/net/virtio/virtio_rxtx.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c index 41a1366..9d2f7d6 100644 --- a/drivers/net/virtio/virtio_rxtx.c +++ b/drivers/net/virtio/virtio_rxtx.c @@ -571,9 +571,6 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts) if (likely(num > DESC_PER_CACHELINE)) num = num - ((rxvq->vq_used_cons_idx + num) % DESC_PER_CACHELINE); - if (num == 0) - return 0; - num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); PMD_RX_LOG(DEBUG, "used:%d dequeue:%d", nb_used, num); @@ -671,9 +668,6 @@ virtio_recv_mergeable_pkts(void *rx_queue, virtio_rmb(); - if (nb_used == 0) - return 0; - PMD_RX_LOG(DEBUG, "used:%d\n", nb_used); hw = rxvq->hw; -- 1.8.3.1
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On 2/23/2016 12:23 AM, Tom Kiely wrote: > Hi, > Sorry I missed the last few messages until now. I'm happy with > just removing the "if". Kyle, when you say you fixed it, do you mean > that you will push the patch or have already done so ? >Thanks, >Tom > > On 02/18/2016 02:03 PM, Kyle Larose wrote: >> On Tue, Jan 5, 2016 at 2:13 AM, Xie, Huawei >> wrote: >>> On 12/17/2015 7:18 PM, Tom Kiely wrote: On 11/25/2015 05:32 PM, Xie, Huawei wrote: > On 11/13/2015 5:33 PM, Tom Kiely wrote: >> If all rx descriptors are processed while transient >> mbuf exhaustion is present, the rx ring ends up with >> no available descriptors. Thus no packets are received >> on that ring. Since descriptor refill is performed post >> rx descriptor processing, in this case no refill is >> ever subsequently performed resulting in permanent rx >> traffic drop. >> >> Signed-off-by: Tom Kiely >> --- >>drivers/net/virtio/virtio_rxtx.c |6 -- >>1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_rxtx.c >> b/drivers/net/virtio/virtio_rxtx.c >> index 5770fa2..a95e234 100644 >> --- a/drivers/net/virtio/virtio_rxtx.c >> +++ b/drivers/net/virtio/virtio_rxtx.c >> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf >> **rx_pkts, uint16_t nb_pkts) >>if (likely(num > DESC_PER_CACHELINE)) >>num = num - ((rxvq->vq_used_cons_idx + num) % >> DESC_PER_CACHELINE); >>-if (num == 0) >> +/* Refill free descriptors even if no pkts recvd */ >> +if (num == 0 && virtqueue_full(rxvq)) > Should the return condition be that no used buffers and we have avail > descs in avail ring, i.e, > num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > > rather than > num == 0 && rxvq->vq_free_cnt == 0 Yes we could do that but I don't see a good reason to wait until the vq_free_cnt == vq_nentries before attempting the refill. The existing code will attempt refill even if only 1 packet was received and the free count is small. To me it seems safer to extend that to try refill even if no packet was received but the free count is non-zero. >>> The existing code attempt to refill only if 1 packet was received. >>> >>> If we want to refill even no packet was received, then the strict >>> condition should be >>> num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries >>> >>> The safer condition, what you want to use, should be >>> num == 0 && !virtqueue_full(...) >>> rather than >>> num == 0 && virtqueue_full(...) >>> >>> We could simplify things a bit, just remove this check, if the >>> following >>> receiving code already takes care of the "num == 0" condition. >>> >> FWIW, I fixed this issue myself by just removing the if(num == 0) >> checks entirely. I didn't see any benefit in short-circuiting a loop >> which pretty much does nothing anyway when num == 0. Further, we only >> hit this case when there's no packets to receive, which means there's >> probably a few cycles to spare. This is even simpler. Yes, as i said, that is the simplest fix. >> >>> I find virtqueue_full is confusing, maybe we could change it to some >>> other meaningful name. >>> Tom >>return 0; >> num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); >> @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, >> virtio_rmb(); >>-if (nb_used == 0) >> +/* Refill free descriptors even if no pkts recvd */ >> +if (nb_used == 0 && virtqueue_full(rxvq)) >>return 0; >> PMD_RX_LOG(DEBUG, "used:%d\n", nb_used); > >
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
Hi, Sorry I missed the last few messages until now. I'm happy with just removing the "if". Kyle, when you say you fixed it, do you mean that you will push the patch or have already done so ? Thanks, Tom On 02/18/2016 02:03 PM, Kyle Larose wrote: > On Tue, Jan 5, 2016 at 2:13 AM, Xie, Huawei wrote: >> On 12/17/2015 7:18 PM, Tom Kiely wrote: >>> >>> On 11/25/2015 05:32 PM, Xie, Huawei wrote: On 11/13/2015 5:33 PM, Tom Kiely wrote: > If all rx descriptors are processed while transient > mbuf exhaustion is present, the rx ring ends up with > no available descriptors. Thus no packets are received > on that ring. Since descriptor refill is performed post > rx descriptor processing, in this case no refill is > ever subsequently performed resulting in permanent rx > traffic drop. > > Signed-off-by: Tom Kiely > --- >drivers/net/virtio/virtio_rxtx.c |6 -- >1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/virtio/virtio_rxtx.c > b/drivers/net/virtio/virtio_rxtx.c > index 5770fa2..a95e234 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, uint16_t nb_pkts) >if (likely(num > DESC_PER_CACHELINE)) >num = num - ((rxvq->vq_used_cons_idx + num) % > DESC_PER_CACHELINE); >-if (num == 0) > +/* Refill free descriptors even if no pkts recvd */ > +if (num == 0 && virtqueue_full(rxvq)) Should the return condition be that no used buffers and we have avail descs in avail ring, i.e, num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries rather than num == 0 && rxvq->vq_free_cnt == 0 >>> Yes we could do that but I don't see a good reason to wait until the >>> vq_free_cnt == vq_nentries >>> before attempting the refill. The existing code will attempt refill >>> even if only 1 packet was received >>> and the free count is small. To me it seems safer to extend that to >>> try refill even if no packet was received >>> but the free count is non-zero. >> The existing code attempt to refill only if 1 packet was received. >> >> If we want to refill even no packet was received, then the strict >> condition should be >> num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries >> >> The safer condition, what you want to use, should be >> num == 0 && !virtqueue_full(...) >> rather than >> num == 0 && virtqueue_full(...) >> >> We could simplify things a bit, just remove this check, if the following >> receiving code already takes care of the "num == 0" condition. >> > FWIW, I fixed this issue myself by just removing the if(num == 0) > checks entirely. I didn't see any benefit in short-circuiting a loop > which pretty much does nothing anyway when num == 0. Further, we only > hit this case when there's no packets to receive, which means there's > probably a few cycles to spare. This is even simpler. > >> I find virtqueue_full is confusing, maybe we could change it to some >> other meaningful name. >> >>> Tom >>> >return 0; > num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); > @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, > virtio_rmb(); >-if (nb_used == 0) > +/* Refill free descriptors even if no pkts recvd */ > +if (nb_used == 0 && virtqueue_full(rxvq)) >return 0; > PMD_RX_LOG(DEBUG, "used:%d\n", nb_used); >>>
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On Tue, Jan 5, 2016 at 2:13 AM, Xie, Huawei wrote: > On 12/17/2015 7:18 PM, Tom Kiely wrote: >> >> >> On 11/25/2015 05:32 PM, Xie, Huawei wrote: >>> On 11/13/2015 5:33 PM, Tom Kiely wrote: If all rx descriptors are processed while transient mbuf exhaustion is present, the rx ring ends up with no available descriptors. Thus no packets are received on that ring. Since descriptor refill is performed post rx descriptor processing, in this case no refill is ever subsequently performed resulting in permanent rx traffic drop. Signed-off-by: Tom Kiely --- drivers/net/virtio/virtio_rxtx.c |6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c index 5770fa2..a95e234 100644 --- a/drivers/net/virtio/virtio_rxtx.c +++ b/drivers/net/virtio/virtio_rxtx.c @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts) if (likely(num > DESC_PER_CACHELINE)) num = num - ((rxvq->vq_used_cons_idx + num) % DESC_PER_CACHELINE); -if (num == 0) +/* Refill free descriptors even if no pkts recvd */ +if (num == 0 && virtqueue_full(rxvq)) >>> Should the return condition be that no used buffers and we have avail >>> descs in avail ring, i.e, >>> num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries >>> >>> rather than >>> num == 0 && rxvq->vq_free_cnt == 0 >> Yes we could do that but I don't see a good reason to wait until the >> vq_free_cnt == vq_nentries >> before attempting the refill. The existing code will attempt refill >> even if only 1 packet was received >> and the free count is small. To me it seems safer to extend that to >> try refill even if no packet was received >> but the free count is non-zero. > The existing code attempt to refill only if 1 packet was received. > > If we want to refill even no packet was received, then the strict > condition should be > num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > > The safer condition, what you want to use, should be > num == 0 && !virtqueue_full(...) > rather than > num == 0 && virtqueue_full(...) > > We could simplify things a bit, just remove this check, if the following > receiving code already takes care of the "num == 0" condition. > FWIW, I fixed this issue myself by just removing the if(num == 0) checks entirely. I didn't see any benefit in short-circuiting a loop which pretty much does nothing anyway when num == 0. Further, we only hit this case when there's no packets to receive, which means there's probably a few cycles to spare. This is even simpler. > I find virtqueue_full is confusing, maybe we could change it to some > other meaningful name. > >> >>Tom >> return 0; num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, virtio_rmb(); -if (nb_used == 0) +/* Refill free descriptors even if no pkts recvd */ +if (nb_used == 0 && virtqueue_full(rxvq)) return 0; PMD_RX_LOG(DEBUG, "used:%d\n", nb_used); >> >> >
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On Tue, Jan 05, 2016 at 07:13:04AM +, Xie, Huawei wrote: > On 12/17/2015 7:18 PM, Tom Kiely wrote: > > > > > > On 11/25/2015 05:32 PM, Xie, Huawei wrote: > >> On 11/13/2015 5:33 PM, Tom Kiely wrote: > >>> If all rx descriptors are processed while transient > >>> mbuf exhaustion is present, the rx ring ends up with > >>> no available descriptors. Thus no packets are received > >>> on that ring. Since descriptor refill is performed post > >>> rx descriptor processing, in this case no refill is > >>> ever subsequently performed resulting in permanent rx > >>> traffic drop. > >>> > >>> Signed-off-by: Tom Kiely > >>> --- > >>> drivers/net/virtio/virtio_rxtx.c |6 -- > >>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/virtio/virtio_rxtx.c > >>> b/drivers/net/virtio/virtio_rxtx.c > >>> index 5770fa2..a95e234 100644 > >>> --- a/drivers/net/virtio/virtio_rxtx.c > >>> +++ b/drivers/net/virtio/virtio_rxtx.c > >>> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf > >>> **rx_pkts, uint16_t nb_pkts) > >>> if (likely(num > DESC_PER_CACHELINE)) > >>> num = num - ((rxvq->vq_used_cons_idx + num) % > >>> DESC_PER_CACHELINE); > >>> -if (num == 0) > >>> +/* Refill free descriptors even if no pkts recvd */ > >>> +if (num == 0 && virtqueue_full(rxvq)) > >> Should the return condition be that no used buffers and we have avail > >> descs in avail ring, i.e, > >> num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > >> > >> rather than > >> num == 0 && rxvq->vq_free_cnt == 0 > > Yes we could do that but I don't see a good reason to wait until the > > vq_free_cnt == vq_nentries > > before attempting the refill. The existing code will attempt refill > > even if only 1 packet was received > > and the free count is small. To me it seems safer to extend that to > > try refill even if no packet was received > > but the free count is non-zero. > The existing code attempt to refill only if 1 packet was received. > > If we want to refill even no packet was received, then the strict > condition should be > num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > > The safer condition, what you want to use, should be > num == 0 && !virtqueue_full(...) > rather than > num == 0 && virtqueue_full(...) > > We could simplify things a bit, just remove this check, if the following > receiving code already takes care of the "num == 0" condition. > > I find virtqueue_full is confusing, maybe we could change it to some > other meaningful name. > > > > >Tom > > Ping. Tom and Huawei, what is the status of this patch? Will there be a V2? /Bruce
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On 12/17/2015 7:18 PM, Tom Kiely wrote: > > > On 11/25/2015 05:32 PM, Xie, Huawei wrote: >> On 11/13/2015 5:33 PM, Tom Kiely wrote: >>> If all rx descriptors are processed while transient >>> mbuf exhaustion is present, the rx ring ends up with >>> no available descriptors. Thus no packets are received >>> on that ring. Since descriptor refill is performed post >>> rx descriptor processing, in this case no refill is >>> ever subsequently performed resulting in permanent rx >>> traffic drop. >>> >>> Signed-off-by: Tom Kiely >>> --- >>> drivers/net/virtio/virtio_rxtx.c |6 -- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/virtio/virtio_rxtx.c >>> b/drivers/net/virtio/virtio_rxtx.c >>> index 5770fa2..a95e234 100644 >>> --- a/drivers/net/virtio/virtio_rxtx.c >>> +++ b/drivers/net/virtio/virtio_rxtx.c >>> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf >>> **rx_pkts, uint16_t nb_pkts) >>> if (likely(num > DESC_PER_CACHELINE)) >>> num = num - ((rxvq->vq_used_cons_idx + num) % >>> DESC_PER_CACHELINE); >>> -if (num == 0) >>> +/* Refill free descriptors even if no pkts recvd */ >>> +if (num == 0 && virtqueue_full(rxvq)) >> Should the return condition be that no used buffers and we have avail >> descs in avail ring, i.e, >> num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries >> >> rather than >> num == 0 && rxvq->vq_free_cnt == 0 > Yes we could do that but I don't see a good reason to wait until the > vq_free_cnt == vq_nentries > before attempting the refill. The existing code will attempt refill > even if only 1 packet was received > and the free count is small. To me it seems safer to extend that to > try refill even if no packet was received > but the free count is non-zero. The existing code attempt to refill only if 1 packet was received. If we want to refill even no packet was received, then the strict condition should be num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries The safer condition, what you want to use, should be num == 0 && !virtqueue_full(...) rather than num == 0 && virtqueue_full(...) We could simplify things a bit, just remove this check, if the following receiving code already takes care of the "num == 0" condition. I find virtqueue_full is confusing, maybe we could change it to some other meaningful name. > >Tom > >>> return 0; >>> num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); >>> @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, >>> virtio_rmb(); >>> -if (nb_used == 0) >>> +/* Refill free descriptors even if no pkts recvd */ >>> +if (nb_used == 0 && virtqueue_full(rxvq)) >>> return 0; >>> PMD_RX_LOG(DEBUG, "used:%d\n", nb_used); > >
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On 11/25/2015 05:32 PM, Xie, Huawei wrote: > On 11/13/2015 5:33 PM, Tom Kiely wrote: >> If all rx descriptors are processed while transient >> mbuf exhaustion is present, the rx ring ends up with >> no available descriptors. Thus no packets are received >> on that ring. Since descriptor refill is performed post >> rx descriptor processing, in this case no refill is >> ever subsequently performed resulting in permanent rx >> traffic drop. >> >> Signed-off-by: Tom Kiely >> --- >> drivers/net/virtio/virtio_rxtx.c |6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_rxtx.c >> b/drivers/net/virtio/virtio_rxtx.c >> index 5770fa2..a95e234 100644 >> --- a/drivers/net/virtio/virtio_rxtx.c >> +++ b/drivers/net/virtio/virtio_rxtx.c >> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf >> **rx_pkts, uint16_t nb_pkts) >> if (likely(num > DESC_PER_CACHELINE)) >> num = num - ((rxvq->vq_used_cons_idx + num) % >> DESC_PER_CACHELINE); >> >> -if (num == 0) >> +/* Refill free descriptors even if no pkts recvd */ >> +if (num == 0 && virtqueue_full(rxvq)) > Should the return condition be that no used buffers and we have avail > descs in avail ring, i.e, > num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > > rather than > num == 0 && rxvq->vq_free_cnt == 0 Yes we could do that but I don't see a good reason to wait until the vq_free_cnt == vq_nentries before attempting the refill. The existing code will attempt refill even if only 1 packet was received and the free count is small. To me it seems safer to extend that to try refill even if no packet was received but the free count is non-zero. Tom >> return 0; >> >> num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); >> @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, >> >> virtio_rmb(); >> >> -if (nb_used == 0) >> +/* Refill free descriptors even if no pkts recvd */ >> +if (nb_used == 0 && virtqueue_full(rxvq)) >> return 0; >> >> PMD_RX_LOG(DEBUG, "used:%d\n", nb_used);
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
Hi, Sorry for the delay. I have been occupied on another critical issue. I'll look at this today. Tom On 12/17/2015 04:47 AM, Xie, Huawei wrote: > On 11/26/2015 1:33 AM, Xie, Huawei wrote: >> On 11/13/2015 5:33 PM, Tom Kiely wrote: >>> If all rx descriptors are processed while transient >>> mbuf exhaustion is present, the rx ring ends up with >>> no available descriptors. Thus no packets are received >>> on that ring. Since descriptor refill is performed post >>> rx descriptor processing, in this case no refill is >>> ever subsequently performed resulting in permanent rx >>> traffic drop. >>> >>> Signed-off-by: Tom Kiely >>> --- >>> drivers/net/virtio/virtio_rxtx.c |6 -- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/virtio/virtio_rxtx.c >>> b/drivers/net/virtio/virtio_rxtx.c >>> index 5770fa2..a95e234 100644 >>> --- a/drivers/net/virtio/virtio_rxtx.c >>> +++ b/drivers/net/virtio/virtio_rxtx.c >>> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf >>> **rx_pkts, uint16_t nb_pkts) >>> if (likely(num > DESC_PER_CACHELINE)) >>> num = num - ((rxvq->vq_used_cons_idx + num) % >>> DESC_PER_CACHELINE); >>> >>> - if (num == 0) >>> + /* Refill free descriptors even if no pkts recvd */ >>> + if (num == 0 && virtqueue_full(rxvq)) >> Should the return condition be that no used buffers and we have avail >> descs in avail ring, i.e, >> num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries >> >> rather than >> num == 0 && rxvq->vq_free_cnt == 0 >> ? > Tom: > Any further progress? >>> return 0; >>> >>> num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); >>> @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, >>> >>> virtio_rmb(); >>> >>> - if (nb_used == 0) >>> + /* Refill free descriptors even if no pkts recvd */ >>> + if (nb_used == 0 && virtqueue_full(rxvq)) >>> return 0; >>> >>> PMD_RX_LOG(DEBUG, "used:%d\n", nb_used);
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On 11/26/2015 1:33 AM, Xie, Huawei wrote: > On 11/13/2015 5:33 PM, Tom Kiely wrote: >> If all rx descriptors are processed while transient >> mbuf exhaustion is present, the rx ring ends up with >> no available descriptors. Thus no packets are received >> on that ring. Since descriptor refill is performed post >> rx descriptor processing, in this case no refill is >> ever subsequently performed resulting in permanent rx >> traffic drop. >> >> Signed-off-by: Tom Kiely >> --- >> drivers/net/virtio/virtio_rxtx.c |6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_rxtx.c >> b/drivers/net/virtio/virtio_rxtx.c >> index 5770fa2..a95e234 100644 >> --- a/drivers/net/virtio/virtio_rxtx.c >> +++ b/drivers/net/virtio/virtio_rxtx.c >> @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf >> **rx_pkts, uint16_t nb_pkts) >> if (likely(num > DESC_PER_CACHELINE)) >> num = num - ((rxvq->vq_used_cons_idx + num) % >> DESC_PER_CACHELINE); >> >> -if (num == 0) >> +/* Refill free descriptors even if no pkts recvd */ >> +if (num == 0 && virtqueue_full(rxvq)) > Should the return condition be that no used buffers and we have avail > descs in avail ring, i.e, > num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries > > rather than > num == 0 && rxvq->vq_free_cnt == 0 > ? Tom: Any further progress? >> return 0; >> >> num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); >> @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, >> >> virtio_rmb(); >> >> -if (nb_used == 0) >> +/* Refill free descriptors even if no pkts recvd */ >> +if (nb_used == 0 && virtqueue_full(rxvq)) >> return 0; >> >> PMD_RX_LOG(DEBUG, "used:%d\n", nb_used); >
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On 11/13/2015 5:33 PM, Tom Kiely wrote: > If all rx descriptors are processed while transient > mbuf exhaustion is present, the rx ring ends up with > no available descriptors. Thus no packets are received > on that ring. Since descriptor refill is performed post > rx descriptor processing, in this case no refill is > ever subsequently performed resulting in permanent rx > traffic drop. > > Signed-off-by: Tom Kiely > --- > drivers/net/virtio/virtio_rxtx.c |6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/virtio/virtio_rxtx.c > b/drivers/net/virtio/virtio_rxtx.c > index 5770fa2..a95e234 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, uint16_t nb_pkts) > if (likely(num > DESC_PER_CACHELINE)) > num = num - ((rxvq->vq_used_cons_idx + num) % > DESC_PER_CACHELINE); > > - if (num == 0) > + /* Refill free descriptors even if no pkts recvd */ > + if (num == 0 && virtqueue_full(rxvq)) Should the return condition be that no used buffers and we have avail descs in avail ring, i.e, num == 0 && rxvq->vq_free_cnt != rxvq->vq_nentries rather than num == 0 && rxvq->vq_free_cnt == 0 ? > return 0; > > num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); > @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, > > virtio_rmb(); > > - if (nb_used == 0) > + /* Refill free descriptors even if no pkts recvd */ > + if (nb_used == 0 && virtqueue_full(rxvq)) > return 0; > > PMD_RX_LOG(DEBUG, "used:%d\n", nb_used);
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On Tue, Nov 24, 2015 at 10:20:22PM +0100, Thomas Monjalon wrote: > Any review, please? Huawei, would you review it? Sorry that I've not read too much code about virtio PMD driver yet. --yliu > 2015-11-13 09:30, Tom Kiely: > > If all rx descriptors are processed while transient > > mbuf exhaustion is present, the rx ring ends up with > > no available descriptors. Thus no packets are received > > on that ring. Since descriptor refill is performed post > > rx descriptor processing, in this case no refill is > > ever subsequently performed resulting in permanent rx > > traffic drop. > > > > Signed-off-by: Tom Kiely
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
On 11/25/2015 9:47 AM, Yuanhan Liu wrote: > On Tue, Nov 24, 2015 at 10:20:22PM +0100, Thomas Monjalon wrote: >> Any review, please? > Huawei, would you review it? Sorry that I've not read too much > code about virtio PMD driver yet. Np. will do it by end of this week. > > --yliu > > >> 2015-11-13 09:30, Tom Kiely: >>> If all rx descriptors are processed while transient >>> mbuf exhaustion is present, the rx ring ends up with >>> no available descriptors. Thus no packets are received >>> on that ring. Since descriptor refill is performed post >>> rx descriptor processing, in this case no refill is >>> ever subsequently performed resulting in permanent rx >>> traffic drop. >>> >>> Signed-off-by: Tom Kiely
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
Any review, please? 2015-11-13 09:30, Tom Kiely: > If all rx descriptors are processed while transient > mbuf exhaustion is present, the rx ring ends up with > no available descriptors. Thus no packets are received > on that ring. Since descriptor refill is performed post > rx descriptor processing, in this case no refill is > ever subsequently performed resulting in permanent rx > traffic drop. > > Signed-off-by: Tom Kiely
[dpdk-dev] [PATCH] virtio: fix rx ring descriptor starvation
If all rx descriptors are processed while transient mbuf exhaustion is present, the rx ring ends up with no available descriptors. Thus no packets are received on that ring. Since descriptor refill is performed post rx descriptor processing, in this case no refill is ever subsequently performed resulting in permanent rx traffic drop. Signed-off-by: Tom Kiely --- drivers/net/virtio/virtio_rxtx.c |6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c index 5770fa2..a95e234 100644 --- a/drivers/net/virtio/virtio_rxtx.c +++ b/drivers/net/virtio/virtio_rxtx.c @@ -586,7 +586,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts) if (likely(num > DESC_PER_CACHELINE)) num = num - ((rxvq->vq_used_cons_idx + num) % DESC_PER_CACHELINE); - if (num == 0) + /* Refill free descriptors even if no pkts recvd */ + if (num == 0 && virtqueue_full(rxvq)) return 0; num = virtqueue_dequeue_burst_rx(rxvq, rcv_pkts, len, num); @@ -683,7 +684,8 @@ virtio_recv_mergeable_pkts(void *rx_queue, virtio_rmb(); - if (nb_used == 0) + /* Refill free descriptors even if no pkts recvd */ + if (nb_used == 0 && virtqueue_full(rxvq)) return 0; PMD_RX_LOG(DEBUG, "used:%d\n", nb_used); -- 1.7.10.4
