DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback
Date: Fri, 13 Mar 2015 14:50:03 +0000	[thread overview]
Message-ID: <20150313145002.GA11352@bricha3-MOBL3> (raw)
In-Reply-To: <20150313134514.GC28191@hmsreliant.think-freely.org>

On Fri, Mar 13, 2015 at 09:45:14AM -0400, Neil Horman wrote:
> On Fri, Mar 13, 2015 at 09:41:33AM +0000, Bruce Richardson wrote:
> > On Thu, Mar 12, 2015 at 03:15:40PM -0400, Neil Horman wrote:
> > > On Thu, Mar 12, 2015 at 04:54:27PM +0000, John McNamara wrote:
> > > > 
> > > > This patch is a minor extension to the recent patchset for RX/TX callbacks
> > > > based on feedback from users implementing solutions based on it.
> > > > 
> > > > The patch adds a new parameter to the RX callback to pass in the number of
> > > > available RX packets in addition to the number of dequeued packets.
> > > > This provides the RX callback functions with additional information
> > > > that can be used to decide how packets from a burst are handled.
> > > > 
> > > > The TX callback doesn't require this additional parameter so the RX
> > > > and TX callbacks no longer have the same function parameters. As such
> > > > the single RX/TX callback has been refactored into two separate callbacks.
> > > > 
> > > > Since this is an API change we hope it can be included in 2.0.0 to avoid
> > > > changing the API in a subsequent release.    
> > > > 
> > > > 
> > > > John McNamara (1):
> > > >   ethdev: added additional packet count parameter to RX callbacks
> > > > 
> > > >  examples/rxtx_callbacks/main.c |    3 +-
> > > >  lib/librte_ether/rte_ethdev.c  |    8 ++--
> > > >  lib/librte_ether/rte_ethdev.h  |   74 ++++++++++++++++++++++++++--------------
> > > >  3 files changed, 54 insertions(+), 31 deletions(-)
> > > > 
> > > > -- 
> > > > 1.7.4.1
> > > > 
> > > > 
> > > 
> > > 
> > > Well, we're well past the new feature phase of this cycle, so I would say NACK.
> > > I would also suggest that you don't need to modify ABI to accomodate this
> > > feature.  Instead just document the pkts array to be terminated by a reserved
> > > value, so that the callback can determine its size dynamically.  You could
> > > alternatively create a new api call that allows you to retrieve that information
> > > from the context of the callback.
> > > 
> > > Neil
> > > 
> > 
> > Yes, I would agree we are past the new feature phase. However, given that we
> > are making a change to the API, and a fairly small change too - adding one extra
> > parameter - we think that the benefit of including this now outweighs any risk
> > of merging the patch. It seems a bit crazy to ship a release with a new API and
> > then immediately change the API straight after release. Is it not better to
> > take the received feedback on the API and fix/improve it pre-release before it
> > gets set-in-stone?
> > 
> > /Bruce
> > 
> > 
> 
> See above, the API doesn't need to change at all to accomodate this as far as I
> can see.
> 
> Neil
Yes, there are alternative ways to see about accomplishing the same thing, but
they are not nearly as clear as adding in the extra param. That's why we'd like
to see this change go in before release, if possible. If it doesn't make 2.0
I'd like to see it in 2.1, but at the cost of having an API change and the
additional versionning and deprecation that ensues.

Regards,
/Bruce

  reply	other threads:[~2015-03-13 14:50 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-12 16:54 John McNamara
2015-03-12 16:54 ` [dpdk-dev] [PATCH] ethdev: added additional packet count parameter to RX callbacks John McNamara
2015-03-12 19:15 ` [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback Neil Horman
2015-03-12 23:24   ` Mcnamara, John
2015-03-13  9:41   ` Bruce Richardson
2015-03-13 13:45     ` Neil Horman
2015-03-13 14:50       ` Bruce Richardson [this message]
2015-03-13 15:09         ` Neil Horman
2015-03-13 16:26           ` Mcnamara, John
2015-03-13 17:31             ` Neil Horman
2015-03-13 18:28               ` Mcnamara, John
2015-03-13 23:15                 ` Neil Horman
2015-03-23 15:16                   ` Thomas Monjalon
2015-03-23 15:29                     ` Bruce Richardson
2015-03-23 16:00                     ` Neil Horman
2015-03-30 19:52                       ` Thomas Monjalon

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=20150313145002.GA11352@bricha3-MOBL3 \
    --to=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=nhorman@tuxdriver.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).