DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Richardson, Bruce" <bruce.richardson@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Neil Horman <nhorman@tuxdriver.com>,
	"Liang, Cunming" <cunming.liang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet
Date: Wed, 22 Oct 2014 15:09:36 +0000
Message-ID: <59AF69C657FD0841A61C55336867B5B034421C3A@IRSMSX103.ger.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB977258213955A6@IRSMSX105.ger.corp.intel.com>



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Wednesday, October 22, 2014 3:53 PM
> To: Neil Horman; Liang, Cunming
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx
> cycles/packet
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> > Sent: Wednesday, October 22, 2014 3:03 PM
> > To: Liang, Cunming
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx
> cycles/packet
> >
> > On Tue, Oct 21, 2014 at 01:17:01PM +0000, Liang, Cunming wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > Sent: Tuesday, October 21, 2014 6:33 PM
> > > > To: Liang, Cunming
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx
> > > > cycles/packet
> > > >
> > > > On Sun, Oct 12, 2014 at 11:10:39AM +0000, Liang, Cunming wrote:
> > > > > Hi Neil,
> > > > >
> > > > > Very appreciate your comments.
> > > > > I add inline reply, will send v3 asap when we get alignment.
> > > > >
> > > > > BRs,
> > > > > Liang Cunming
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > > > Sent: Saturday, October 11, 2014 1:52 AM
> > > > > > To: Liang, Cunming
> > > > > > Cc: dev@dpdk.org
> > > > > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx
> > > > cycles/packet
> > > > > >
<...snip...>
> > > > > >
> > > > > > > +		printf("Force Stop!\n");
> > > > > > > +		stop = 1;
> > > > > > > +	}
> > > > > > > +	if (signum == SIGUSR2)
> > > > > > > +		stats_display(0);
> > > > > > > +}
> > > > > > > +/* main processing loop */
> > > > > > > +static int
> > > > > > > +main_loop(__rte_unused void *args)
> > > > > > > +{
> > > > > > > +#define PACKET_SIZE 64
> > > > > > > +#define FRAME_GAP 12
> > > > > > > +#define MAC_PREAMBLE 8
> > > > > > > +	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
> > > > > > > +	unsigned lcore_id;
> > > > > > > +	unsigned i, portid, nb_rx = 0, nb_tx = 0;
> > > > > > > +	struct lcore_conf *conf;
> > > > > > > +	uint64_t prev_tsc, cur_tsc;
> > > > > > > +	int pkt_per_port;
> > > > > > > +	uint64_t packets_per_second, total_packets;
> > > > > > > +
> > > > > > > +	lcore_id = rte_lcore_id();
> > > > > > > +	conf = &lcore_conf[lcore_id];
> > > > > > > +	if (conf->status != LCORE_USED)
> > > > > > > +		return 0;
> > > > > > > +
> > > > > > > +	pkt_per_port =  MAX_TRAFIC_BURST / conf->nb_ports;
> > > > > > > +
> > > > > > > +	int idx = 0;
> > > > > > > +	for (i = 0; i < conf->nb_ports; i++) {
> > > > > > > +		int num = pkt_per_port;
> > > > > > > +		portid = conf->portlist[i];
> > > > > > > +		printf("inject %d packet to port %d\n", num, portid);
> > > > > > > +		while (num) {
> > > > > > > +			nb_tx = RTE_MIN(MAX_PKT_BURST, num);
> > > > > > > +			nb_tx = rte_eth_tx_burst(portid, 0,
> > > > > > > +						&tx_burst[idx], nb_tx);
> > > > > > > +			num -= nb_tx;
> > > > > > > +			idx += nb_tx;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +	printf("Total packets inject to prime ports = %u\n", idx);
> > > > > > > +
> > > > > > > +	packets_per_second = (link_mbps * 1000 * 1000) /
> > > > > > > +		+((PACKET_SIZE + FRAME_GAP + MAC_PREAMBLE) *
> CHAR_BIT);
> > > > > > > +	printf("Each port will do %"PRIu64" packets per second\n",
> > > > > > > +		+packets_per_second);
> > > > > > > +
> > > > > > > +	total_packets = RTE_TEST_DURATION * conf->nb_ports *
> > > > > > packets_per_second;
> > > > > > > +	printf("Test will stop after at least %"PRIu64" packets
> received\n",
> > > > > > > +		+ total_packets);
> > > > > > > +
> > > > > > > +	prev_tsc = rte_rdtsc();
> > > > > > > +
> > > > > > > +	while (likely(!stop)) {
> > > > > > > +		for (i = 0; i < conf->nb_ports; i++) {
> > > > > > > +			portid = conf->portlist[i];
> > > > > > > +			nb_rx = rte_eth_rx_burst((uint8_t) portid, 0,
> > > > > > > +						 pkts_burst,
> MAX_PKT_BURST);
> > > > > > > +			if (unlikely(nb_rx == 0)) {
> > > > > > > +				idle++;
> > > > > > > +				continue;
> > > > > > > +			}
> > > > > > > +
> > > > > > > +			count += nb_rx;
> > > > > > Doesn't take into consideration error conditions.  rte_eth_rx_burst can
> > > > return
> > > > > > -ENOTSUP
> > > > > [Liang, Cunming] It returns -ENOTSUP when turning on ETHDEV_DEBUG
> > > > CONFIG.
> > > > > The error is used to avoid no function call register.
> > > > > When ETHDEV_DEBUG turn off, the NULL function call cause segfault
> directly.
> > > > > So I think it's a library internal error.
> > > > No, look at FUNC_PTR_OR_ERR_RET.  If the passed in function pointer is
> null,
> > > > -ENOTSUPP will be returned to the application, you need to handle the
> error
> > > > condition.
> > > [Liang, Cunming] The runtime rte_eth_rx_burst is a inline function in
> rte_ethdev.h.
> > > The one you're talking about is the one defined in rte_ethdev.c which is a
> extern non-inline function.
> > > It works only when RTE_LIBRTE_ETHDEV_DEBUG turns on.
> > > If we always turns such library internal checking on, it lose performance.
> > > So I insist it's a library internal error checking, doesn't need to take care in
> application level.
> > I'm really flored that you would respond this way.
> >
> > What you have is two versions of a function, one returns errors and one
> doesn't,
> > and the version used is based on compile time configuration.  You've written
> > your application such that it can only gracefully handle one of the two
> > versions, and you're happy to allow the other version, the one thats supposed
> to
> > be in use when you are debugging applications, to create silent accounting
> > failures in your application.  And it completely ignores the fact that the
> > non-debug version might one day return errors as well (even if it doesn't
> > currently).  Your assertion above that we can ignore it because its debug code
> > is the most short sighted thing I've read in a long time.
> 
> Actually looking at rte_eth_rx_burst(): it returns uint16_t.
> Which is sort of a strange if it expects to return a negative value as error code.
> Also reading though 'formal' comments of rte_eth_rx_burst() - it seems that it
> not supposed to return negative values:
> "...
> The rte_eth_rx_burst() function does not provide any error
>  notification to avoid the corresponding overhead. As a hint, the
>  upper-level application might check the status of the device link once
>  being systematically returned a 0 value for a given number of tries.
> ...
> @return
>  *   The number of packets actually retrieved, which is the number
>  *   of pointers to *rte_mbuf* structures effectively supplied to the
>  *   *rx_pkts* array."
> 
> Again, if you looks at rte_eth_rx_burst() implementation, when
> RTE_LIBRTE_ETHDEV_DEBUG is on -
> For some error cases it returns '0', foor other's: -ENOTSUP:
> 
> FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, -ENOTSUP);
> if (queue_id >= dev->data->nb_rx_queues) {
>                 PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n", queue_id);
>                 return 0;
> }
> 
> Which makes me thing that we just have errnoneous implementation of
> rte_eth_rx_burst()
> when RTE_LIBRTE_ETHDEV_DEBUG is on.
> Probably just a mechanical mistake, when FUNC_PTR_OR_ERR_RET() was
> added.
>  I'd say we change rte_eth_rx_burst() to always return 0 (as was initially
> planned).
> 
> Konstantin

Agreed. For any errors other than no-packets available yet, we can also consider setting rte_errno when returning 0. 

/Bruce

  reply	other threads:[~2014-10-22 15:02 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-25  6:12 [dpdk-dev] [PATCH 0/5] app/test: unit test to measure cycles per packet Cunming Liang
2014-08-25  6:12 ` [dpdk-dev] [PATCH 1/5] app/test: unit test for rx and tx cycles/packet Cunming Liang
2014-08-25  6:12 ` [dpdk-dev] [PATCH 2/5] app/test: measure standalone rx or " Cunming Liang
2014-08-25  6:12 ` [dpdk-dev] [PATCH 3/5] ixgbe/vpmd: add fix to store unaligned mbuf point array Cunming Liang
2014-08-25  6:12 ` [dpdk-dev] [PATCH 4/5] app/test: add unit test to measure RX burst cycles Cunming Liang
2014-08-25  6:12 ` [dpdk-dev] [PATCH 5/5] app/test: allow to create packets in different sizes Cunming Liang
2014-09-03  1:46 ` [dpdk-dev] [PATCH 0/5] app/test: unit test to measure cycles per packet Zhan, Zhaochen
2014-10-10 12:29 ` [dpdk-dev] [PATCH v2 0/4] " Cunming Liang
2014-10-10 12:29   ` [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet Cunming Liang
2014-10-10 17:52     ` Neil Horman
2014-10-12 11:10       ` Liang, Cunming
     [not found]         ` <E115CCD9D858EF4F90C690B0DCB4D8972262B720@IRSMSX108.ger.corp.intel.com>
2014-10-14  0:54           ` Liang, Cunming
2014-10-21 10:33         ` Neil Horman
2014-10-21 10:43           ` Richardson, Bruce
2014-10-21 13:37             ` Neil Horman
2014-10-21 22:43               ` Ananyev, Konstantin
2014-10-21 13:17           ` Liang, Cunming
2014-10-22 14:03             ` Neil Horman
2014-10-22 14:48               ` Liang, Cunming
2014-10-22 14:53               ` Ananyev, Konstantin
2014-10-22 15:09                 ` Richardson, Bruce [this message]
2014-10-24  3:06                   ` Liang, Cunming
2014-10-10 12:29   ` [dpdk-dev] [PATCH v2 2/4] app/test: measure standalone rx or " Cunming Liang
2014-10-10 12:30   ` [dpdk-dev] [PATCH v2 3/4] app/test: add unit test to measure RX burst cycles Cunming Liang
2014-10-10 12:30   ` [dpdk-dev] [PATCH v2 4/4] app/test: allow to create packets in different sizes Cunming Liang
2014-10-20  8:13   ` [dpdk-dev] [PATCH v3 0/2] app/test: unit test to measure cycles per packet Cunming Liang
2014-10-20  8:13     ` [dpdk-dev] [PATCH v3 1/2] app/test: allow to create packets in different sizes Cunming Liang
2014-10-20  8:13     ` [dpdk-dev] [PATCH v3 2/2] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang
2014-10-21  2:40     ` [dpdk-dev] [PATCH v3 0/2] app/test: unit test to measure cycles per packet Liu, Yong
2014-10-24  5:39     ` [dpdk-dev] [PATCH v4 0/3] " Cunming Liang
2014-10-24  5:39       ` [dpdk-dev] [PATCH v4 1/3] app/test: allow to create packets in different sizes Cunming Liang
2014-10-24  5:39       ` [dpdk-dev] [PATCH v4 2/3] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang
2014-10-24  5:39       ` [dpdk-dev] [PATCH v4 3/3] ethdev: fix wrong error return refer to API definition Cunming Liang
2014-10-24  5:57       ` [dpdk-dev] [PATCH v5 0/3] app/test: unit test to measure cycles per packet Cunming Liang
2014-10-24  5:58         ` [dpdk-dev] [PATCH v5 1/3] app/test: allow to create packets in different sizes Cunming Liang
2014-10-24  5:58         ` [dpdk-dev] [PATCH v5 2/3] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang
2014-10-24  5:58         ` [dpdk-dev] [PATCH v5 3/3] ethdev: fix wrong error return refere to API definition Cunming Liang
2014-10-27  1:20         ` [dpdk-dev] [PATCH v6 0/3] app/test: unit test to measure cycles per packet Cunming Liang
2014-10-27  1:20           ` [dpdk-dev] [PATCH v6 1/3] app/test: allow to create packets in different sizes Cunming Liang
2014-10-27  1:20           ` [dpdk-dev] [PATCH v6 2/3] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang
2014-11-11 23:28             ` Thomas Monjalon
2014-11-12  6:32               ` Liang, Cunming
2014-10-27  1:20           ` [dpdk-dev] [PATCH v6 3/3] ethdev: fix wrong error return refere to API definition Cunming Liang
2014-10-27 16:03             ` Ananyev, Konstantin
2014-10-27  1:45           ` [dpdk-dev] [PATCH v6 0/3] app/test: unit test to measure cycles per packet Liu, Yong
2014-10-29  5:06           ` Liang, Cunming
2014-11-12  6:24           ` [dpdk-dev] [PATCH v7 0/7] " Cunming Liang
2014-11-12  6:24             ` [dpdk-dev] [PATCH v7 1/7] app/test: allow to create packets in different sizes Cunming Liang
2014-11-12  6:24             ` [dpdk-dev] [PATCH v7 2/7] ixgbe:clean scattered_rx configure in dev_stop Cunming Liang
2014-11-12  7:53               ` Thomas Monjalon
2014-11-12  8:21                 ` Liang, Cunming
2014-11-12  9:24                   ` Thomas Monjalon
2014-11-12 10:29                     ` Liang, Cunming
2014-11-12 10:32                       ` Thomas Monjalon
2014-11-12 10:42                         ` Liang, Cunming
2014-11-12  6:24             ` [dpdk-dev] [PATCH v7 3/7] ether: new API to format eth_addr in string Cunming Liang
2014-11-12  6:24             ` [dpdk-dev] [PATCH v7 4/7] app/testpmd: cleanup eth_addr print Cunming Liang
2014-11-12  6:24             ` [dpdk-dev] [PATCH v7 5/7] examples: " Cunming Liang
2014-11-12  6:24             ` [dpdk-dev] [PATCH v7 6/7] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang
2014-11-12  6:24             ` [dpdk-dev] [PATCH v7 7/7] ethdev: fix wrong error return refere to API definition Cunming Liang
2014-11-12 23:50             ` [dpdk-dev] [PATCH v7 0/7] app/test: unit test to measure cycles per packet Thomas Monjalon
2014-10-24  5:59       ` [dpdk-dev] [PATCH v4 0/3] " Liang, Cunming
     [not found]       ` <1414130090-17910-1-git-send-email-y>
     [not found]         ` <1414130090-17910-4-git-send-email-y>
2014-10-24 11:04           ` [dpdk-dev] [PATCH v5 3/3] ethdev: fix wrong error return refere to API definition Ananyev, Konstantin
2014-10-27  0:58             ` Liang, Cunming
2014-10-28 12:21   ` [dpdk-dev] [PATCH v2 0/4] app/test: unit test to measure cycles per packet Neil Horman

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=59AF69C657FD0841A61C55336867B5B034421C3A@IRSMSX103.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=cunming.liang@intel.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --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

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