DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Morten Brørup <mb@smartsharesystems.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	arybchenko@solarflare.com, jia.guo@intel.com, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] ethdev: rte_eth_rx_burst()nb_pktsrequirements
Date: Wed, 14 Oct 2020 12:38:55 +0100
Message-ID: <20201014113855.GG1513@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C6136C@smartserver.smartshare.dk>

On Wed, Oct 14, 2020 at 01:14:13PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Wednesday, October 14, 2020 11:30 AM
> > 
> > On Wed, Oct 14, 2020 at 10:53:24AM +0200, Thomas Monjalon wrote:
> > > 14/10/2020 10:26, Morten Brørup:
> > > > From: Ferruh Yigit
> > > > > On 9/14/2020 1:42 PM, Morten Brørup wrote:
> > > > > > From: Bruce Richardson
> > > > > >> On Mon, Sep 14, 2020 at 01:05:11PM +0200, Morten Brørup wrote:
> > > > > >>> Updated description of rte_eth_rx_burst() to reflect what
> > drivers,
> > > > > >>> when using vector instructions, expect from nb_pkts.
> > > > > >>>
> > > > > >>> Also discussed on the mailing list here:
> > > > > >>>
> > > > > >>
> > http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35C61257@smarts
> > > > > >> erver.smartshare.dk/
> > > > > >>>
> > > > > >>> --- a/lib/librte_ethdev/rte_ethdev.h
> > > > > >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> > > > > >>> @@ -4469,6 +4469,10 @@ int
> > > > > >> rte_eth_dev_hairpin_capability_get(uint16_t port_id,
> > > > > >>>    * burst-oriented optimizations in both synchronous and
> > asynchronous
> > > > > >>>    * packet processing environments with no overhead in both
> > cases.
> > > > > >>>    *
> > > > > >>> + * @note
> > > > > >>> + *   Some drivers using vector instructions require that
> > *nb_pkts*
> > > > > >> is
> > > > > >>> + *   divisible by 4 or 8, depending on the driver
> > implementation.
> > > > > >>> + *
> > > > > >>
> > > > > >> Not technically true, in that the drivers will round the value
> > down to
> > > > > >> the
> > > > > >> nearest multiple of 4 or 8. So how about rewording as:
> > > > > >>
> > > > > >> "Some drivers using vector instructions may round the *nb_pkts*
> > driver
> > > > > >> to
> > > > > >> a multiple of 4 or 8 depending upon the driver implementation."
> > > > > >>
> > > > > >
> > > > > > You are correct about the driver behavior.
> > > > > >
> > > > > > However, if you pass nb_pkts=9, the driver will return 8 packets,
> > > > > > and thus it does not conform to the API behavior of returning
> > nb_pkts
> > > > > > if they are there.
> > > > > >
> > > > > > This is why the description in this patch differs from the
> > description we
> > > > > reached in the RFC discussion.
> > > > > >
> > > > >
> > > > > Hi Morten, Bruce,
> > > > >
> > > > > +1 to document the this behavior.
> > > > >
> > > > > But in the patch the wording is more strict:
> > > > > "... require that *nb_pkts* is divisible by 4 or 8 ..."
> > > > > "... The value must be divisible by 8 in order to work with any
> > driver."
> > > > >
> > > > > I am not sure the requirement is that strict. Application still
> > provide any
> > > > > value for 'nb_pkts', so the value doesn't "have to" be divisible 8/4.
> > > > >
> > > > > But for vector PMD case it will return number of packets round down
> > to 8/4.
> > > > > Perhaps can add for vector PMD it must be at least 4/8?
> > > > >
> > > > > Bruce's explanation sound more accurate to me, what do you think?
> > > > >
> > > >
> > > > I aim to keep the explanation in the documentation relatively simple.
> > Keep the parameter description short, and add the details about vector
> > driver behavior as a note to the function.
> > > >
> > > > The reason for all this is the existing documentation describing how to
> > use the rte_eth_rx_burst() function at high level:
> > > >
> > > > The rte_eth_rx_burst() function returns the number of packets actually
> > retrieved [...]. A return value equal to nb_pkts indicates [...] 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.
> > > >
> > > > As an alternative to my proposed solution, we could add that vector
> > drivers round down to 4 or 8, and the application's comparison of the
> > nb_pkts and return value must consider this. But that would make the above
> > description strangely complex, rather than just requiring that nb_pkts for
> > vector drivers must be divisible by 4 or 8.
> > > >
> > > > And as a minor detail, keeping my proposed restriction would also
> > eliminate the vector drivers' need to round down.
> > > >
> > > > I don't see a need to be able to call rte_eth_rx_burst() with a value
> > not divisible by 4 or 8 for a vector driver, so my proposed restriction is
> > a tradeoff favoring simplicity over unnecessary flexibility.
> > >
> > > It makes sense to me.
> > >
> > 
> > That sounds reasonable for what we have now. We just need to standardize on
> > either 4 or 8 as the required factor of the input size. I would suggest
> > having it as 4, and look to put in fallback paths for the few drivers which
> > don't support less than 8. I think that 8 is too large a min burst size to
> > support, for any apps that want small bursts for lower latency.
> > 
> 
> 8 works with all drivers today, so I prefer 8. Also, it seems more future proof for even higher bandwidths. If you need low latency, don't use vector drivers. Also, DPDK generally seems to prefer throughput over latency, so 8 seems like the better choice to me.
> 
> Bruce, if you have a very strong preference for standardizing on 4 and you can convince the affected driver developers to modify their drivers to support 4, we can go ahead with that.
> 
Let's go with 8 for now, and we can reduce in future if we can.

  reply	other threads:[~2020-10-14 11:39 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-14 11:05 [dpdk-dev] [PATCH] ethdev: rte_eth_rx_burst() nb_pkts requirements Morten Brørup
2020-09-14 11:26 ` Bruce Richardson
2020-09-14 12:42   ` [dpdk-dev] [PATCH] ethdev: rte_eth_rx_burst() nb_pktsrequirements Morten Brørup
2020-10-13 10:55     ` Ferruh Yigit
2020-10-14  8:26       ` [dpdk-dev] [PATCH] ethdev: rte_eth_rx_burst()nb_pktsrequirements Morten Brørup
2020-10-14  8:53         ` Thomas Monjalon
2020-10-14  9:29           ` Bruce Richardson
2020-10-14 11:14             ` Morten Brørup
2020-10-14 11:38               ` Bruce Richardson [this message]
2020-10-15  7:36                 ` Morten Brørup
2020-10-26 15:06                 ` [dpdk-dev] [PATCH] ethdev: rte_eth_rx_burst() nb_pkts requirements Morten Brørup
2020-10-26 15:15 ` Ajit Khaparde
2020-10-27 10:24 ` Bruce Richardson
2020-10-30  9:27   ` Ferruh Yigit

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=20201014113855.GG1513@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jia.guo@intel.com \
    --cc=mb@smartsharesystems.com \
    --cc=thomas@monjalon.net \
    /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

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