From mboxrd@z Thu Jan 1 00:00:00 1970 From: MAC Lee Subject: =?utf-8?b?5Zue6KaG77iwICBbUEFUQ0hdIFRoZSBWTVhORVQzIFBN?= =?utf-8?q?D_can=27t_receive_packet_suddenly_after_a_lot_of_traffic_coming?= =?utf-8?q?_in=2E_The_root_cause_is_due_to_mbuf_allocation_fail_in_vmxnet3?= =?utf-8?q?=5Fpost=5Frx=5Fbufs=28=29_and_there_is_no_error_handling_when_i?= =?utf-8?q?t_is_called_from_vmxnet3=5Frecv=5Fpkts=28=29=2E_The_RXD_will_no?= =?utf-8?q?t_have_=22free=22_mbuf_for_it_but_the_counter_still_increment?= =?utf-8?q?=2E_Finally=2C_no_packet_can_be_received=2E_This_fix_is_allocat?= =?utf-8?q?e_the_mbuf_first=2C_if_the_allocation_is_failed=2C_then_don=27t?= =?utf-8?q?_receive_any_packet_and_the_packet_will_remain_in_RXD_to_preven?= =?utf-8?q?t_any_packet_drop=2EIf_the_allocation_is_sucess=2C_the_vmxnet3?= =?utf-8?b?X3Bvc3RfcnhfYnVmcygpIHdpbGwgY2FsbCB2bXhuZXQzX3JlbmV3X2Rlc2Mo?= =?utf-8?q?=29_and_RXD_will_be_renew_inside=2E?= Date: Thu, 23 Jul 2015 05:44:27 +0000 (UTC) Message-ID: <1629788597.582932.1437630267877.JavaMail.yahoo@mail.yahoo.com> References: <20150722222731.497b2abc@uryu.home.lan> Reply-To: MAC Lee Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Stephen Hemminger Return-path: Received: from nm33-vm10.bullet.mail.sg3.yahoo.com (nm33-vm10.bullet.mail.sg3.yahoo.com [106.10.151.233]) by dpdk.org (Postfix) with ESMTP id 7FA465947 for ; Thu, 23 Jul 2015 07:44:30 +0200 (CEST) In-Reply-To: <20150722222731.497b2abc@uryu.home.lan> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Stephen,=C2=A0 =C2=A0 I have question about point 1 and want to discuss = with you. Why it will cause deadlock if just stop receiving it in this case= s? =C2=A0And I also found VMXNET3 PMD have the same bug in RX. I will rewor= k the patch and submit later. Thanks! Best Regards,Marco=20 Stephen Hemminger =E6=96=BC 2015=E5=B9=B4= 07=E6=9C=8823=E6=97=A5 (=E9=80=B1=E5=9B=9B) 1:27 PM =E5=AF=AB=E9=81=93=EF= =B9=95 =20 On Thu, 23 Jul 2015 09:48:55 +0800 mac_leehk@yahoo.com.hk wrote: > From: marco Thank you for addressing a real bug.=20 But there are several issues with the patch as submitted: * the standard way to handle allocation failure in network drivers is to d= rop the =C2=A0 received packet and reuse the available data buffer (mbuf) for the n= ext packet. =C2=A0 It looks like your code would just stop receiving which could cause = deadlock. * the mail is formatted in a manner than is incompatible with merging into= git. =C2=A0 All submissions should have a short < 60 character Subject with a su= mmary =C2=A0 followed by a description.=C2=A0 I don't know what mail client you u= sed but everything =C2=A0 is smashed into the Subject. * all patches require a Signed-off-by with a real name for Developer's Cer= tificate Of Origin * the style is wrong, indentation is a mess please indent with tabs not sp= aces. * avoid extra comments, often in code too many comments are worse than too= few Please rework your patch and resubmit it. >=C2=A0 drivers/net/vmxnet3/vmxnet3_rxtx.c |=C2=A0 54 +++++++++++++++++++++= ++++++++++++++- >=C2=A0 1 file changed, 53 insertions(+), 1 deletion(-) >=C2=A0 mode change 100644 =3D> 100755 drivers/net/vmxnet3/vmxnet3_rxtx.c >=20 > diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmx= net3_rxtx.c > old mode 100644 > new mode 100755 > index 39ad6ef..d560bbb > --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c > +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c > @@ -421,6 +421,51 @@ vmxnet3_xmit_pkts(void *tx_queue, struct rte_mbuf **= tx_pkts, >=C2=A0 =C2=A0=C2=A0=C2=A0 return nb_tx; >=C2=A0 } >=C2=A0=20 > +static inline void > +vmxnet3_renew_desc(vmxnet3_rx_queue_t *rxq, uint8_t ring_id,struct rte_m= buf *mbuf) > +{ > +=C2=A0=C2=A0=C2=A0 uint32_t=C2=A0 val =3D 0; > +=C2=A0=C2=A0=C2=A0 struct vmxnet3_cmd_ring *ring =3D &rxq->cmd_ring[ring= _id]; > + > +=C2=A0=C2=A0=C2=A0 struct Vmxnet3_RxDesc *rxd; > +=C2=A0=C2=A0=C2=A0 vmxnet3_buf_info_t *buf_info =3D &ring->buf_info[ring= ->next2fill]; > + > +=C2=A0=C2=A0=C2=A0 rxd =3D (struct Vmxnet3_RxDesc *)(ring->base + ring->= next2fill); > + > +=C2=A0=C2=A0=C2=A0 if (ring->rid =3D=3D 0) { > +=C2=A0=C2=A0=C2=A0 /* Usually: One HEAD type buf per packet > +=C2=A0=C2=A0=C2=A0 * val =3D (ring->next2fill % rxq->hw->bufs_per_pkt) ? > +=C2=A0=C2=A0=C2=A0 * VMXNET3_RXD_BTYPE_BODY : VMXNET3_RXD_BTYPE_HEAD; > +=C2=A0=C2=A0=C2=A0 */ > + > +=C2=A0=C2=A0=C2=A0 /* We use single packet buffer so all heads here */ > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 val =3D VMXNET3_RXD_BTYPE_HEAD; > +=C2=A0=C2=A0=C2=A0 } else { > +=C2=A0=C2=A0=C2=A0 /* All BODY type buffers for 2nd ring; which won't be= used at all by ESXi */ > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 val =3D VMXNET3_RXD_BTYPE_BODY; > +=C2=A0=C2=A0=C2=A0 } > + > +=C2=A0=C2=A0=C2=A0 /* > +=C2=A0=C2=A0=C2=A0 * Load mbuf pointer into buf_info[ring_size] > +=C2=A0=C2=A0=C2=A0 * buf_info structure is equivalent to cookie for virt= io-virtqueue > +=C2=A0=C2=A0=C2=A0 */ > +=C2=A0=C2=A0=C2=A0 buf_info->m =3D mbuf; > +=C2=A0=C2=A0=C2=A0 buf_info->len =3D (uint16_t)(mbuf->buf_len - > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2= =A0 =C2=A0=C2=A0=C2=A0 =C2=A0 RTE_PKTMBUF_HEADROOM); > +=C2=A0=C2=A0=C2=A0 buf_info->bufPA =3D RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb= uf); > + > +=C2=A0=C2=A0=C2=A0 /* Load Rx Descriptor with the buffer's GPA */ > +=C2=A0=C2=A0=C2=A0 rxd->addr =3D buf_info->bufPA; > + > +=C2=A0=C2=A0=C2=A0 /* After this point rxd->addr MUST not be NULL */ > +=C2=A0=C2=A0=C2=A0 rxd->btype =3D val; > +=C2=A0=C2=A0=C2=A0 rxd->len =3D buf_info->len; > +=C2=A0=C2=A0=C2=A0 /* Flip gen bit at the end to change ownership */ > +=C2=A0=C2=A0=C2=A0 rxd->gen =3D ring->gen; > + > +=C2=A0=C2=A0=C2=A0 vmxnet3_cmd_ring_adv_next2fill(ring); > + > +} >=C2=A0 /* >=C2=A0 *=C2=A0 Allocates mbufs and clusters. Post rx descriptors with buff= er details >=C2=A0 *=C2=A0 so that device can receive packets in those buffers. > @@ -575,8 +620,15 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf **= rx_pkts, uint16_t nb_pkts) >=C2=A0 =C2=A0=C2=A0=C2=A0 } >=C2=A0=20 >=C2=A0 =C2=A0=C2=A0=C2=A0 while (rcd->gen =3D=3D rxq->comp_ring.gen) { > +=C2=A0=C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct rte_mbuf *rep; >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 if (nb_rx >=3D nb_pkts) >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 break; > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=20 > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 rep =3D rte_rxmbuf_alloc(rxq->mp); > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0 if (rep =3D=3D NULL) { > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2= =A0=C2=A0 rxq->stats.rx_buf_alloc_failure++; > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2= =A0=C2=A0 break; > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0=C2=A0 } >=C2=A0=20 >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 idx =3D rcd->rxdIdx; >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 ring_idx =3D (uint8_t)((rcd->= rqID =3D=3D rxq->qid1) ? 0 : 1); > @@ -657,7 +709,7 @@ rcd_done: >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 VMXNET3_INC_RING_IDX_ONLY(rxq= ->cmd_ring[ring_idx].next2comp, rxq->cmd_ring[ring_idx].size); >=C2=A0=20 >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 /* It's time to allocate some= new buf and renew descriptors */ > -=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 vmxnet3_post_rx_bufs(rxq, ring_idx= ); > +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 vmxnet3_renew_desc(rxq, ring_idx,r= ep); >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 if (unlikely(rxq->shared->ctr= l.updateRxProd)) { >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 VMXNET3_WR= ITE_BAR0_REG(hw, rxprod_reg[ring_idx] + (rxq->queue_id * VMXNET3_REG_ALIGN)= , >=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2= =A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 rxq->cmd_ring[ring_idx].n= ext2fill);