From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 6A96D5F3B; Mon, 25 Jun 2018 09:55:58 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jun 2018 00:55:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,269,1526367600"; d="scan'208";a="240343963" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 25 Jun 2018 00:55:57 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 25 Jun 2018 00:55:57 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 25 Jun 2018 00:55:57 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.51]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.82]) with mapi id 14.03.0319.002; Mon, 25 Jun 2018 15:55:55 +0800 From: "Zhang, Qi Z" To: "Zhao1, Wei" , "dev@dpdk.org" CC: "Lu, Wenzhuo" , "stable@dpdk.org" Thread-Topic: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error Thread-Index: AQHUCgdbURLlU+aZWUajP7ZpLhIudKRsRLawgANw9YCAAJDFwP//wLcAgACP0SD//4HmAIAAhmBw Date: Mon, 25 Jun 2018 07:55:54 +0000 Message-ID: <039ED4275CED7440929022BC67E706115323C866@SHSMSX103.ccr.corp.intel.com> References: <1529656727-40207-1-git-send-email-wei.zhao1@intel.com> <039ED4275CED7440929022BC67E706115323BAD2@SHSMSX103.ccr.corp.intel.com> <039ED4275CED7440929022BC67E706115323C621@SHSMSX103.ccr.corp.intel.com> <039ED4275CED7440929022BC67E706115323C83C@SHSMSX103.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjkwNGFhMmUtNjQ5NC00NGIyLWEwOTEtYmJlZTZkNjcyYWViIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiQjBKMHgwKzNLaHVydlMzVWlLY2hJaXloZ3FCd0pONUIrY2Y4VkttV2c2RXVLM1phZHRsZHI5akpYTlNBV3NPUCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix Tx check descriptor status APIs error X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Jun 2018 07:55:59 -0000 > -----Original Message----- > From: Zhao1, Wei > Sent: Monday, June 25, 2018 3:53 PM > To: Zhang, Qi Z ; dev@dpdk.org > Cc: Lu, Wenzhuo ; stable@dpdk.org > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs error >=20 > Hi, qi >=20 > > -----Original Message----- > > From: Zhang, Qi Z > > Sent: Monday, June 25, 2018 3:47 PM > > To: Zhao1, Wei ; dev@dpdk.org > > Cc: Lu, Wenzhuo ; stable@dpdk.org > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs > > error > > > > > > > > > -----Original Message----- > > > From: Zhao1, Wei > > > Sent: Monday, June 25, 2018 2:49 PM > > > To: Zhang, Qi Z ; dev@dpdk.org > > > Cc: Lu, Wenzhuo ; stable@dpdk.org > > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status APIs > > > error > > > > > > Hi=1B$B!$=1B(Bqi > > > > > > > -----Original Message----- > > > > From: Zhang, Qi Z > > > > Sent: Monday, June 25, 2018 10:49 AM > > > > To: Zhao1, Wei ; dev@dpdk.org > > > > Cc: Lu, Wenzhuo ; stable@dpdk.org > > > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status > > > > APIs error > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Zhao1, Wei > > > > > Sent: Monday, June 25, 2018 9:58 AM > > > > > To: Zhang, Qi Z ; dev@dpdk.org > > > > > Cc: Lu, Wenzhuo ; stable@dpdk.org > > > > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status > > > > > APIs error > > > > > > > > > > Hi, > > > > > > > > > > > -----Original Message----- > > > > > > From: Zhang, Qi Z > > > > > > Sent: Friday, June 22, 2018 9:47 PM > > > > > > To: Zhao1, Wei ; dev@dpdk.org > > > > > > Cc: Lu, Wenzhuo ; stable@dpdk.org > > > > > > Subject: RE: [PATCH] net/ixgbe: fix Tx check descriptor status > > > > > > APIs error > > > > > > > > > > > > Hi Wei: > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Zhao1, Wei > > > > > > > Sent: Friday, June 22, 2018 4:39 PM > > > > > > > To: dev@dpdk.org > > > > > > > Cc: Lu, Wenzhuo ; Zhang, Qi Z > > > > > > > ; stable@dpdk.org; Zhao1, Wei > > > > > > > > > > > > > > Subject: [PATCH] net/ixgbe: fix Tx check descriptor status > > > > > > > APIs error > > > > > > > > > > > > > > This is a issue involve RS bit set rule in ixgbe. > > > > > > > Let us take function ixgbe_xmit_pkts_vec () as an example, > > > > > > > in this function RS bit will be set for descriptor with > > > > > > > index > > > > > > > txq->tx_next_rs, and also descriptor free function > > > > > > > ixgbe_tx_free_bufs() also check RS bit for descriptor with > > > > > > > index > > > > > > > txq->tx_next_rs, This is perfect ok. Let us take an example, > > > > > > > if app set tx_rs_thresh =3D 32 and nb_desc =3D 512, then ixgb= e > > > > > > > PMD code will init > > > > > > > txq->tx_next_rs =3D 31 in function ixgbe_reset_tx_queue when > > > > > > > txq->tx queue > > > > > > setup. > > > > > > > And also txq->tx_next_rs will be update as 63, 95 and so on. > > > > > > > But, in the function ixgbe_dev_tx_descriptor_status(), the > > > > > > > RS bit to check is " desc =3D ((desc > > > > > > > + txq->tx_rs_thresh - 1) / > > > > > > > txq->tx_rs_thresh) * txq-tx_rs_thresh", which is 32 ,64, 96 > > > > > > > txq->and so > > on. > > > > > > > So, they are all wrong! In tx function of > > > > > > > ixgbe_xmit_pkts_simple, the RS bit rule is also the same, it > > > > > > > also set > > > index 31 ,64, 95. > > > > > > > we need to correct it. > > > > > > > > > > > > One question: > > > > > > does only the descriptor with RS bit will have DD status, or > > > > > > NIC will always update all descriptor's DD status but this > > > > > > happens when the next descriptor with RS bit has been sent? > > > > > > If it is the first case, I think you fix still have problem, > > > > > > because multi-seg mbuf or tso offload will break the 31, 63, > > > > > > 95 pattern > > > > > > See: > > > > > > nb_used =3D (uint16_t)(tx_pkt- > > > > > > >nb_segs + new_ctx); > > > > > > > > > > > > if (txp !=3D NULL && > > > > > > nb_used + txq->nb_tx_used >=3D > > > > > txq->tx_rs_thresh) > > > > > > /* set RS on the previous packet in > > > > > > the burst > > > */ > > > > > > txp->read.cmd_type_len |=3D > > > > > > > > > > > rte_cpu_to_le_32(IXGBE_TXD_CMD_RS); > > > > > > > > > > > > so the possible solution is store each RS position in a list > > > > > > at tx, and find the next RS from the list in > > > > > > ixgbe_dev_tx_descriptor_status > > > > > > > > > > > > If it is the second case, it will be simple we don't need to > > > > > > check forward with tx_rs_thresh, just check the exact position > > > > > > ( I hope it is this case :)) > > > > > > > > > > In this patch, code "desc =3D txq->sw_ring[desc].last_id;" will > > > > > get the last index for several segments packet, that solve the > > > > > case when packet contain more than one segment. > > > > > > > > Yes, but my understanding is we "set RS on the previous packet" > > > > but not the packet cross tx_rs_thresh boundary So even without > > > > multi-seg , it will be 30, 62, 94, but not 31, 63, 95, probably > > > > the reason we didn't see the issue, is because if we test it with > > > > 32 burst, the latest packet still will be set RS, so it will be > > > > 30,31, 62,63, 94, 95, but if we tested with 64 burst, I assume it w= ill be 30, > 62, 63, 94 ... right? > > > > > > > > > > Update to last mail. > > > There are another RS bit set code, which set RS bit on last > > > descriptor of the threshold packet. > > > So, that is to say ixgbe_xmit_pkts() not only set 30 62 94, but also = 31 63 95. > > > And it also set the last packet of the burst, so we do not need fix > > > this function, it is not bug. > > > > > > /* Set RS bit only on threshold packets' last descriptor */ > > > if (txq->nb_tx_used >=3D txq->tx_rs_thresh) { > > > PMD_TX_FREE_LOG(DEBUG, > > > "Setting RS bit on TXD id=3D" > > > "%4u (port=3D%d queue=3D%d)", > > > tx_last, txq->port_id, txq->queue_id); > > > > > > cmd_type_len |=3D IXGBE_TXD_CMD_RS; > > > > > > /* Update txq RS bit counters */ > > > txq->nb_tx_used =3D 0; > > > txp =3D NULL; > > > } > > > > OK, but is this guarantee that slot 31, 63, 95 is always in the packet > > that cross tx_rx_thresh boundary? > > Let's assume every packet has 5 seg, so in every 7 packet the last one > > will cross the boundary. > > so it will be > > 0-4, 5-9, 10-14, 15-19, 20-24, 25-29, 30-34, 35-39, 40-44, 45-49, > > 50-54, 55-59, 60-64, 65-69 Definitely, it is possible that last packet > > (and the previous before > > last) does not include 32*n-1 In ixgbe_xmit_cleanup, it use > > desc_to_clean_to =3D sw_ring[desc_to_clean_to].last_id + tx_rx_thresh t= o > > calculate next packet in boundary, that's no problem But in > > ixgbe_dev_tx_descriptor_status, we assume the it is 31,63,95 pattern, t= hat > will be problem. >=20 > In my patch for ixgbe_dev_tx_descriptor_status() We have code : >=20 > desc =3D txq->sw_ring[desc].last_id; > status =3D &txq->tx_ring[desc].wb.status; >=20 > it will only check last_id DD, if in the several segment case, it will al= so to check > the last DD. Yes, but what I mean is we can't guarantee a packet cover 31, 63, 95 is the= packet that cross tx_rs_thresh boundary,(whose last segment has RS). >=20 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > Qi > > > > > > > > > > > > > > Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status > > > > > > > API") > > > > > > > > > > > > > > Signed-off-by: Wei Zhao > > > > > > > --- > > > > > > > drivers/net/ixgbe/ixgbe_rxtx.c | 12 ++++++------ > > > > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c > > > > > > > b/drivers/net/ixgbe/ixgbe_rxtx.c index 3e13d26..f185219 > > > > > > > 100644 > > > > > > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c > > > > > > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c > > > > > > > @@ -3146,15 +3146,15 @@ ixgbe_dev_tx_descriptor_status(void > > > > > > *tx_queue, > > > > > > > uint16_t offset) > > > > > > > return -EINVAL; > > > > > > > > > > > > > > desc =3D txq->tx_tail + offset; > > > > > > > + if (desc >=3D txq->nb_tx_desc) > > > > > > > + desc -=3D txq->nb_tx_desc; > > > > > > > /* go to next desc that has the RS bit */ > > > > > > > - desc =3D ((desc + txq->tx_rs_thresh - 1) / txq->tx_rs_thres= h) * > > > > > > > - txq->tx_rs_thresh; > > > > > > > - if (desc >=3D txq->nb_tx_desc) { > > > > > > > + desc =3D (desc / txq->tx_rs_thresh + 1) * > > > > > > > + txq->tx_rs_thresh - 1; > > > > > > > + if (desc >=3D txq->nb_tx_desc) > > > > > > > desc -=3D txq->nb_tx_desc; > > > > > > > - if (desc >=3D txq->nb_tx_desc) > > > > > > > - desc -=3D txq->nb_tx_desc; > > > > > > > - } > > > > > > > > > > > > > > + desc =3D txq->sw_ring[desc].last_id; > > > > > > > status =3D &txq->tx_ring[desc].wb.status; > > > > > > > if (*status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD)) > > > > > > > return RTE_ETH_TX_DESC_DONE; > > > > > > > -- > > > > > > > 2.7.5