DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Zhao1, Wei" <wei.zhao1@intel.com>,
	"Guo, Jia" <jia.guo@intel.com>,
	"Beilei Xing" <beilei.xing@intel.com>,
	"Qi Zhang" <qi.z.zhang@intel.com>,
	"Xiao Wang" <xiao.w.wang@intel.com>,
	"Jingjing Wu" <jingjing.wu@intel.com>,
	"Qiming Yang" <qiming.yang@intel.com>,
	"Rosen Xu" <rosen.xu@intel.com>
Cc: <dev@dpdk.org>, "Thomas Monjalon" <thomas@monjalon.net>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"Andrew Rybchenko" <arybchenko@solarflare.com>
Subject: Re: [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API
Date: Mon, 27 Jul 2020 11:42:06 +0200	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C61161@smartserver.smartshare.dk> (raw)
In-Reply-To: <MWHPR11MB1391713F63E7AB62A40E31B7B77D0@MWHPR11MB1391.namprd11.prod.outlook.com>

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> Sent: Saturday, July 18, 2020 5:45 AM
> 
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Zhao1, Wei
> > Sent: Saturday, July 18, 2020 11:32 AM
> > To: Morten Brørup <mb@smartsharesystems.com>; Guo, Jia
> > <jia.guo@intel.com>
> >
> > HI,
> >
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Thursday, July 16, 2020 5:09 PM
> > >
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> > > > Sent: Thursday, July 16, 2020 10:50 AM
> > > >
> > > > Hi,
> > > >
> > > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > > Sent: Thursday, July 16, 2020 12:03 AM
> > > > >
> > > > > Wei, Jeff,
> > > > >
> > > > > For the ixgbe driver using vector functions, i.e.
> > > > ixgbe_recv_pkts_vec(), calling
> > > > > rte_eth_rx_burst() with nb_pkts > RTE_IXGBE_MAX_RX_BURST only
> > > > > returns RTE_IXGBE_MAX_RX_BURST packets. E.g. calling
> > > > > rte_eth_rx_burst() with
> > > > > nb_pkts=64 only returns 32 packets.
> > > > >
> > > > >
> > > > > The API description of rte_eth_rx_burst() says:
> > > > >
> > > > > <quote>
> > > > > The rte_eth_rx_burst() function returns the number of packets
> > > > actually
> > > > > retrieved, which is the number of rte_mbuf data structures
> > > > effectively supplied
> > > > > into the rx_pkts array. A return value equal to nb_pkts
> indicates
> > > > that the RX
> > > > > queue contained at least rx_pkts packets, and this is likely to
> > > > signify that other
> > > > > received packets remain in the input queue. Applications
> > > > > implementing
> > > > a
> > > > > "retrieve as much received packets as possible" policy can
> check
> > > > > this
> > > > specific
> > > > > case and keep invoking the rte_eth_rx_burst() function until a
> > > > > value
> > > > less than
> > > > > nb_pkts is returned.
> > > > > </quote>
> > > > >
> > > > > The driver implementation does not conform to the documented
> > > > > behavior
> > > > for
> > > > > "retrieve as much received packets as possible" applications.
> > > >
> > > > It seems not an issue, this function has comment bellow, it is
> > > > design work in that way.
> > > >
> > > >
> > > > /*
> > > >  * vPMD receive routine, only accept(nb_pkts >=
> > > > RTE_IXGBE_DESCS_PER_LOOP)
> > > >  *
> > > >  * Notice:
> > > >  * - nb_pkts < RTE_IXGBE_DESCS_PER_LOOP, just return no packet
> > > >  * - nb_pkts > RTE_IXGBE_MAX_RX_BURST, only scan
> > > RTE_IXGBE_MAX_RX_BURST
> > > >  *   numbers of DD bit
> > > >  * - floor align nb_pkts to a RTE_IXGBE_DESC_PER_LOOP power-of-
> two
> > > > */
> > > >
> > >
> > > I noticed this already. And yes, ixgbe_recv_pkts_vec() does what
> its
> > > comments says.
> > >
> > > However, when ixgbe_recv_pkts_vec() is used as the driver's
> > > implementation of the rte_eth_rx_burst() function call, the
> > > rte_eth_rx_burst() function does not do what is expected of the
> > rte_eth_rx_burst() function.
> > >
> > > The implementation must conform to the API that it implements.
> > >
> > > If you don't want to update the ixgbe_recv_pkts_vec() function, I
> > > propose that you add a wrapper function that calls
> > > ixgbe_recv_pkts_vec() repeatedly, and use the wrapper function as
> the
> > > implementation of the rte_eth_rx_burst() function.
> 
> A code review will be do for that change, it is need because that is a
> important change.

I looked at it, and it seems that most Intel Ethernet drivers suffer from this bug.

Some of them do not seem simple to fix, e.g. the fm40k behavior depends on the configured rx_free_thresh, which defaults to 32.

I think we can all agree that it is too risky for me to attempt to fix drivers for hardware which I do not have available for testing, so I have created a bug in Bugzilla, and am thus passing the baton to the Intel PMD developer team.

> 
> >
> > Get your point, I know what you need, but is there any risk for
> > ixgbe_recv_pkts_vec? I am not sure.
> > Maybe you can have a try first, if it work well, you can submit a
> patch.
> > What you need is this:
> >
> > uint16_t
> > i40e_recv_scattered_pkts_vec_avx2(void *rx_queue, struct rte_mbuf
> > **rx_pkts,
> > 			     uint16_t nb_pkts)
> > {
> > 	uint16_t retval = 0;
> > 	while (nb_pkts > RTE_I40E_VPMD_RX_BURST) {
> > 		uint16_t burst =
> i40e_recv_scattered_burst_vec_avx2(rx_queue,
> > 				rx_pkts + retval, RTE_I40E_VPMD_RX_BURST);
> > 		retval += burst;
> > 		nb_pkts -= burst;
> > 		if (burst < RTE_I40E_VPMD_RX_BURST)
> > 			return retval;
> > 	}
> > 	return retval + i40e_recv_scattered_burst_vec_avx2(rx_queue,
> > 				rx_pkts + retval, nb_pkts);
> > }


  reply	other threads:[~2020-07-27  9:42 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-15 16:02 Morten Brørup
2020-07-16  8:49 ` Zhao1, Wei
2020-07-16  9:08   ` [dpdk-dev] ixgbe vector rx does not conform torte_eth_rx_burst() API Morten Brørup
2020-07-17 13:49     ` Bruce Richardson
2020-07-18  3:32     ` Zhao1, Wei
2020-07-18  3:44       ` Zhao1, Wei
2020-07-27  9:42         ` Morten Brørup [this message]
2020-07-27 10:35           ` [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API Zhao1, Wei

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98CBD80474FA8B44BF855DF32C47DC35C61161@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=arybchenko@solarflare.com \
    --cc=beilei.xing@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jia.guo@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=qi.z.zhang@intel.com \
    --cc=qiming.yang@intel.com \
    --cc=rosen.xu@intel.com \
    --cc=thomas@monjalon.net \
    --cc=wei.zhao1@intel.com \
    --cc=xiao.w.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).