[dpdk-dev] [PATCH v5 1/3] examples/eventdev_pipeline: added sample app

[Jerin Jacob]

-----Original Message-----
> Date: Wed, 5 Jul 2017 12:15:51 +0100
> From: "Hunt, David" <david.hunt at intel.com>
> To: Jerin Jacob <jerin.jacob at caviumnetworks.com>
> CC: dev at dpdk.org, harry.van.haaren at intel.com, Gage Eads
>  <gage.eads at intel.com>, Bruce Richardson <bruce.richardson at intel.com>
> Subject: Re: [PATCH v5 1/3] examples/eventdev_pipeline: added sample app
> User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101
>  Thunderbird/45.8.0
> 
> Hi Jerin,
> 
> 
> On 5/7/2017 6:30 AM, Jerin Jacob wrote:
> > -----Original Message-----
> > > Date: Tue, 4 Jul 2017 08:55:25 +0100
> > > From: "Hunt, David" <david.hunt at intel.com>
> > > To: Jerin Jacob <jerin.jacob at caviumnetworks.com>
> > > CC: dev at dpdk.org, harry.van.haaren at intel.com, Gage Eads
> > >   <gage.eads at intel.com>, Bruce Richardson <bruce.richardson at intel.com>
> > > Subject: Re: [PATCH v5 1/3] examples/eventdev_pipeline: added sample app
> > > User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101
> > >   Thunderbird/45.8.0
> > > 
> > > Hi Jerin,
> > Hi David,
> > 
> > I have checked the v6. Some comments below.
> > 
> > > 
> > > On 3/7/2017 4:57 AM, Jerin Jacob wrote:
> > > > -----Original Message-----
> > > > 
> --snip--
> > > > > +#
> > > > > +#   Redistribution and use in source and binary forms, with or without
> > > > > +#   modification, are permitted provided that the following conditions
> > > > > +#   are met:
> > > > > +#
> > > > > +
> > > > > +static unsigned int active_cores;
> > > > > +static unsigned int num_workers;
> > > > > +static long num_packets = (1L << 25); /* do ~32M packets */
> > > > > +static unsigned int num_fids = 512;
> > > > > +static unsigned int num_stages = 1;
> > > > > +static unsigned int worker_cq_depth = 16;
> > Any reason not move this to struct config_data?
> 
> Sure. All vars now moved to either config_data or fastpath_data.
> 
> > > > > +static int queue_type = RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY;
> > > > > +static int16_t next_qid[MAX_NUM_STAGES+1] = {-1};
> > > > > +static int16_t qid[MAX_NUM_STAGES] = {-1};
> > > > > +static int worker_cycles;
> > used in fastpath move to struct fastpath_data.
> 
> Performance drops by ~20% when I put these in fastpath_data. No performace
> drop when in config_data.
> I think we need to leaving in config_data for now.

I checked v7 it looks to OK to merge. Can you fix following minor issue with
check patch and check-git-log.sh

check-git-log.sh
-----------------
Wrong headline lowercase:
	doc: add sw eventdev pipeline to sample app ug

### examples/eventdev_pipeline: added sample app

Note:
Change application to new name.

checkpatch.sh
-----------------

WARNING:EMBEDDED_FUNCTION_NAME: Prefer using '"%s...", __func__' to
using 'consumer', this function's name, in a string
#294: FILE: examples/eventdev_pipeline_sw_pmd/main.c:178:
+		printf("# consumer RX=%"PRIu64", time %"PRIu64 "ms, "

WARNING:EMBEDDED_FUNCTION_NAME: Prefer using '"%s...", __func__' to
using 'worker', this function's name, in a string
#453: FILE: examples/eventdev_pipeline_sw_pmd/main.c:337:
+		printf("  worker %u thread done. RX=%zu TX=%zu\n",

total: 0 errors, 2 warnings, 1078 lines checked

I will give pull request Thomas on Friday morning. I will include this change set
in the pull request.

Regarding the performance drop, Can you add __rte_cache_aligned on those
variable which creates regression in moving to rte_malloc area. The
cache line could be shared? If not fixing then its fine we will look into that latter.

and

Can you share your command to test for this regression, I will also try
on x86 box if I get time?


> 
> > > > > +static int enable_queue_priorities;
> > struct config_data ?
> 
> Done.
> 
> > > > > +
> > > > > +struct prod_data {
> > > > > +	uint8_t dev_id;
> > > > > +	uint8_t port_id;
> > > > > +	int32_t qid;
> > > > > +	unsigned int num_nic_ports;
> > > > > +} __rte_cache_aligned;
> > > > > +
> > > > > +struct cons_data {
> > > > > +	uint8_t dev_id;
> > > > > +	uint8_t port_id;
> > > > > +} __rte_cache_aligned;
> > > > > +
> > > > > +static struct prod_data prod_data;
> > > > > +static struct cons_data cons_data;
> > > > > +
> > > > > +struct worker_data {
> > > > > +	uint8_t dev_id;
> > > > > +	uint8_t port_id;
> > > > > +} __rte_cache_aligned;
> > > > > +
> > > > > +static unsigned int *enqueue_cnt;
> > > > > +static unsigned int *dequeue_cnt;
> > > > Not been consumed. Remove it.
> > > Done.
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +
> > > > > +static inline void
> > > > > +work(struct rte_mbuf *m)
> > > > > +{
> > > > > +	struct ether_hdr *eth;
> > > > > +	struct ether_addr addr;
> > > > > +
> > > > > +	/* change mac addresses on packet (to use mbuf data) */
> > > > > +	eth = rte_pktmbuf_mtod(m, struct ether_hdr *);
> > > > > +	ether_addr_copy(&eth->d_addr, &addr);
> > > > > +	ether_addr_copy(&eth->s_addr, &eth->d_addr);
> > > > > +	ether_addr_copy(&addr, &eth->s_addr);
> > > > If it is even number of stages(say 2), Will mac swap be negated? as we are
> > > > swapping on each stage NOT in consumer?
> > > The mac swap is just to touch the mbuf. It does not matter if it is negated.
> > Are you sure or I am missing something here? for example,If I add following piece
> > of code, irrespective number of stages it should send the same packet if
> > source packet is same. Right ?
> > But not happening as expected(Check the src and dest mac address)
> > 
> > stage == 1
> > 00000000: 00 0F B7 11 27 2B 00 0F B7 11 27 2C 08 00 45 00 |....'+....',..E.
> > 00000010: 00 2E 00 00 00 00 04 11 D5 B1 C0 A8 12 02 0E 01 |................
> > 00000020: 00 63 10 00 10 01 00 1A 00 00 61 62 63 64 65 66 |.c........abcdef
> > 00000030: 67 68 69 6A 6B 6C 6D 6E 6F 70 71 72 |  |  |  |  | ghijklmnopqr
> > 
> > stage == 2
> > 00000000: 00 0F B7 11 27 2C 00 0F B7 11 27 2B 08 00 45 00 |....',....'+..E.
> > 00000010: 00 2E 00 00 00 00 04 11 D5 B0 C0 A8 12 03 0E 01 |................
> > 00000020: 00 63 10 00 10 01 00 1A 00 00 61 62 63 64 65 66 |.c........abcdef
> > 00000030: 67 68 69 6A 6B 6C 6D 6E 6F 70 71 72 |  |  |  |  | ghijklmnopqr
> > 
> > 
> > diff --git a/examples/eventdev_pipeline_sw_pmd/main.c b/examples/eventdev_pipeline_sw_pmd/main.c
> > index c62cba2..a7aaf37 100644
> > --- a/examples/eventdev_pipeline_sw_pmd/main.c
> > +++ b/examples/eventdev_pipeline_sw_pmd/main.c
> > @@ -147,8 +147,9 @@ consumer(void)
> >          if (start_time == 0)
> > @@ -157,6 +158,7 @@ consumer(void)
> >          received += n;
> >          for (i = 0; i < n; i++) {
> >                  uint8_t outport = packets[i].mbuf->port;
> > +               rte_pktmbuf_dump(stdout, packets[i].mbuf, 64);
> >                  rte_eth_tx_buffer(outport, 0, fdata->tx_buf[outport],
> >                                  packets[i].mbuf);
> > 
> > Either fix the mac swap properly or remove it.
> 
> Temporary fix added. Now reading in addr and writing it back without
> swapping. Not ideal,
> will probably need more work in the future. Added a FIXME in the code with
> agreement from Jerin.

On a agreement that, Before moving to generic application it has to be
fixed in SW PMD driver or if its specific behavior of SW PMD then it has
to abstracted in proper way in fastpath.