> -----Original Message----- > From: Stefan Puiu [mailto:stefan.puiu at gmail.com] > Sent: Monday, November 14, 2016 2:46 AM > To: dev at dpdk.org > Cc: mac_leehk at yahoo.com.hk; Yong Wang <yongwang at vmware.com>; > Stefan Puiu <stefan.puiu at gmail.com> > Subject: [PATCH] vmxnet3: fix Rx deadlock > > Our use case is that we have an app that needs to keep mbufs around > for a while. We've seen cases when calling vmxnet3_post_rx_bufs() from > vmxet3_recv_pkts(), it might not succeed to add any mbufs to any RX > descriptors (where it returns -err). Since there are no mbufs that the > virtual hardware can use, and since nobody calls > vmxnet3_post_rx_bufs() after that, no packets will be received after
The patch looks good overall. I think a more accurate description is that the particular descriptor's generation bit never got flipped properly when an mbuf failed to be refilled which caused the rx stuck, rather than vmxnet3_post_rx_bufs() not being called afterwards. > this. I call this a deadlock for lack of a better term - the virtual > HW waits for free mbufs, while the app waits for the hardware to > notify it for data. Note that after this, the app can't recover. > > This fix is a rework of this patch by Marco Lee: > https://urldefense.proofpoint.com/v2/url?u=http- > 3A__dpdk.org_dev_patchwork_patch_6575_&d=CwIBAg&c=Sqcl0Ez6M0X8a > eM67LKIiDJAXVeAw-YihVMNtXt- > uEs&r=44mSO5N5yEs4CeCdtQE0xt0F7J0p67_mApYVAzyYms0&m=g2gi3ZErdx > AKGY8d3wbhk2D6TLUVYBs3K- > KMdiJwuvI&s=YLz0Wsl_kQUXPWij82nnO9ROB64AK5ZtDCyUvHuU8jA&e= . I > had to forward port it, > address review comments and also reverted the allocation failure > handing to the first version of the patch s/handing/handling > (https://urldefense.proofpoint.com/v2/url?u=http- > 3A__dpdk.org_ml_archives_dev_2015- > 2DJuly_022079.html&d=CwIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw- > YihVMNtXt- > uEs&r=44mSO5N5yEs4CeCdtQE0xt0F7J0p67_mApYVAzyYms0&m=g2gi3ZErdx > AKGY8d3wbhk2D6TLUVYBs3K- > KMdiJwuvI&s=5HksZV8s99b3jVV7Pea60d18hKqXxp4eRpJWjz6sWLc&e= ), > since that's > the only approach that seems to work, and seems to be what other > drivers are doing (I checked ixgbe and em). Reusing the mbuf that's > getting passed to the application doesn't seem to make sense, and it > was causing weird issues in our app. Also, reusing rxm without > checking if it's NULL could cause the code to crash. > > Signed-off-by: Stefan Puiu <stefan.puiu at gmail.com> > --- > drivers/net/vmxnet3/vmxnet3_rxtx.c | 38 > ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 36 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c > b/drivers/net/vmxnet3/vmxnet3_rxtx.c > index b109168..c9d2488 100644 > --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c > +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c > @@ -518,6 +518,32 @@ > return nb_tx; > } > > +static inline void > +vmxnet3_renew_desc(vmxnet3_rx_queue_t *rxq, uint8_t ring_id, > + struct rte_mbuf *mbuf) Nit: align the params here to be consistent with other functions. > +{ > + uint32_t val = 0; Nit: extra space before "val" > + struct vmxnet3_cmd_ring *ring = &rxq->cmd_ring[ring_id]; > + struct Vmxnet3_RxDesc *rxd = > + (struct Vmxnet3_RxDesc *)(ring->base + ring->next2fill); > + vmxnet3_buf_info_t *buf_info = &ring->buf_info[ring->next2fill]; > + > + if (ring_id == 0) > + val = VMXNET3_RXD_BTYPE_HEAD; > + else > + val = VMXNET3_RXD_BTYPE_BODY; > + > + buf_info->m = mbuf; > + buf_info->len = (uint16_t)(mbuf->buf_len - > RTE_PKTMBUF_HEADROOM); > + buf_info->bufPA = rte_mbuf_data_dma_addr_default(mbuf); > + > + rxd->addr = buf_info->bufPA; > + rxd->btype = val; > + rxd->len = buf_info->len; > + rxd->gen = ring->gen; > + > + vmxnet3_cmd_ring_adv_next2fill(ring); > +} > /* > * Allocates mbufs and clusters. Post rx descriptors with buffer details > * so that device can receive packets in those buffers. > @@ -657,9 +683,17 @@ > } > > while (rcd->gen == rxq->comp_ring.gen) { > + struct rte_mbuf *newm; Nit: add a blank line here. > if (nb_rx >= nb_pkts) > break; > > + newm = rte_mbuf_raw_alloc(rxq->mp); > + if (unlikely(newm == NULL)) { > + PMD_RX_LOG(ERR, "Error allocating mbuf"); > + rxq->stats.rx_buf_alloc_failure++; > + break; > + } > + > idx = rcd->rxdIdx; > ring_idx = (uint8_t)((rcd->rqID == rxq->qid1) ? 0 : 1); > rxd = (Vmxnet3_RxDesc *)rxq->cmd_ring[ring_idx].base + > idx; > @@ -759,8 +793,8 @@ > VMXNET3_INC_RING_IDX_ONLY(rxq- > >cmd_ring[ring_idx].next2comp, > rxq->cmd_ring[ring_idx].size); > > - /* It's time to allocate some new buf and renew descriptors > */ > - vmxnet3_post_rx_bufs(rxq, ring_idx); > + /* It's time to renew descriptors */ Nit: extra space before "renew" > + vmxnet3_renew_desc(rxq, ring_idx, newm); > if (unlikely(rxq->shared->ctrl.updateRxProd)) { > VMXNET3_WRITE_BAR0_REG(hw, > rxprod_reg[ring_idx] + (rxq->queue_id * VMXNET3_REG_ALIGN), > > rxq->cmd_ring[ring_idx].next2fill); > -- > 1.9.1
