DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API
@ 2020-07-15 16:02 Morten Brørup
  2020-07-16  8:49 ` Zhao1, Wei
  0 siblings, 1 reply; 8+ messages in thread
From: Morten Brørup @ 2020-07-15 16:02 UTC (permalink / raw)
  To: Wei Zhao, Jeff Guo; +Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko

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.


Med venlig hilsen / kind regards
- Morten Brørup


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API
  2020-07-15 16:02 [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API 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
  0 siblings, 1 reply; 8+ messages in thread
From: Zhao1, Wei @ 2020-07-16  8:49 UTC (permalink / raw)
  To: Morten Brørup, Guo, Jia
  Cc: dev, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko

Hi, 

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, July 16, 2020 12:03 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
> Subject: ixgbe vector rx does not conform to rte_eth_rx_burst() API
> 
> 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
 */

> 
> 
> Med venlig hilsen / kind regards
> - Morten Brørup


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] ixgbe vector rx does not conform torte_eth_rx_burst() API
  2020-07-16  8:49 ` Zhao1, Wei
@ 2020-07-16  9:08   ` Morten Brørup
  2020-07-17 13:49     ` Bruce Richardson
  2020-07-18  3:32     ` Zhao1, Wei
  0 siblings, 2 replies; 8+ messages in thread
From: Morten Brørup @ 2020-07-16  9:08 UTC (permalink / raw)
  To: Zhao1, Wei, Guo, Jia
  Cc: dev, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> Sent: Thursday, July 16, 2020 10:50 AM
> 
> Hi,
> 
> > -----Original Message-----
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Thursday, July 16, 2020 12:03 AM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> Ferruh
> > <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> > Subject: ixgbe vector rx does not conform to rte_eth_rx_burst() API
> >
> > 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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] ixgbe vector rx does not conform torte_eth_rx_burst() API
  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
  1 sibling, 0 replies; 8+ messages in thread
From: Bruce Richardson @ 2020-07-17 13:49 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Zhao1, Wei, Guo, Jia, dev, Thomas Monjalon, Yigit, Ferruh,
	Andrew Rybchenko

On Thu, Jul 16, 2020 at 11:08:49AM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> > Sent: Thursday, July 16, 2020 10:50 AM
> > 
> > Hi,
> > 
> > > -----Original Message-----
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Thursday, July 16, 2020 12:03 AM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> > > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> > Ferruh
> > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>
> > > Subject: ixgbe vector rx does not conform to rte_eth_rx_burst() API
> > >
> > > 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.
>
I agree it does not conform to the API as documented. Looking at the i40e
AVX2 vector code paths, the regular (no-fragmented packets) path does not
seem to put a limit on the size of burst received, while the scattered
receive path uses the latter scheme of a wrapper function. Therefore,
either approach should work for ixgbe driver, I think.

/Bruce

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] ixgbe vector rx does not conform torte_eth_rx_burst() API
  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
  1 sibling, 1 reply; 8+ messages in thread
From: Zhao1, Wei @ 2020-07-18  3:32 UTC (permalink / raw)
  To: Morten Brørup, Guo, Jia
  Cc: dev, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko

HI, 

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, July 16, 2020 5:09 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@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 torte_eth_rx_burst()
> API
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> > Sent: Thursday, July 16, 2020 10:50 AM
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Thursday, July 16, 2020 12:03 AM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> > > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> > Ferruh
> > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>
> > > Subject: ixgbe vector rx does not conform to rte_eth_rx_burst() API
> > >
> > > 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.

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);
}


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] ixgbe vector rx does not conform torte_eth_rx_burst() API
  2020-07-18  3:32     ` Zhao1, Wei
@ 2020-07-18  3:44       ` Zhao1, Wei
  2020-07-27  9:42         ` [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API Morten Brørup
  0 siblings, 1 reply; 8+ messages in thread
From: Zhao1, Wei @ 2020-07-18  3:44 UTC (permalink / raw)
  To: Zhao1, Wei, Morten Brørup, Guo, Jia
  Cc: dev, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko



> -----Original Message-----
> 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>
> 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 torte_eth_rx_burst()
> API
> 
> HI,
> 
> > -----Original Message-----
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Thursday, July 16, 2020 5:09 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@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
> > torte_eth_rx_burst() API
> >
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> > > Sent: Thursday, July 16, 2020 10:50 AM
> > >
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > Sent: Thursday, July 16, 2020 12:03 AM
> > > > To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> > > > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Yigit,
> > > Ferruh
> > > > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > > <arybchenko@solarflare.com>
> > > > Subject: ixgbe vector rx does not conform to rte_eth_rx_burst()
> > > > API
> > > >
> > > > 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.

> 
> 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);
> }


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API
  2020-07-18  3:44       ` Zhao1, Wei
@ 2020-07-27  9:42         ` Morten Brørup
  2020-07-27 10:35           ` Zhao1, Wei
  0 siblings, 1 reply; 8+ messages in thread
From: Morten Brørup @ 2020-07-27  9:42 UTC (permalink / raw)
  To: Zhao1, Wei, Guo, Jia, Beilei Xing, Qi Zhang, Xiao Wang,
	Jingjing Wu, Qiming Yang, Rosen Xu
  Cc: dev, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko

> 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);
> > }


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API
  2020-07-27  9:42         ` [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API Morten Brørup
@ 2020-07-27 10:35           ` Zhao1, Wei
  0 siblings, 0 replies; 8+ messages in thread
From: Zhao1, Wei @ 2020-07-27 10:35 UTC (permalink / raw)
  To: Morten Brørup, Guo, Jia, Xing, Beilei, Zhang, Qi Z, Wang,
	Xiao W, Wu, Jingjing, Yang, Qiming, Xu, Rosen
  Cc: dev, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko



> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, July 27, 2020 5:42 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wang,
> Xiao W <xiao.w.wang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yang,
> Qiming <qiming.yang@intel.com>; Xu, Rosen <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
> 
> > 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.

Yes

> 
> 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.
> 

Ok.

> >
> > >
> > > 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);
> > > }


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-27 10:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 16:02 [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API 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         ` [dpdk-dev] ixgbe vector rx does not conform to rte_eth_rx_burst() API Morten Brørup
2020-07-27 10:35           ` Zhao1, Wei

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git