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

Hunt, David david.hunt at intel.com
Thu Jul 6 12:04:41 CEST 2017



On 6/7/2017 4:31 AM, Jerin Jacob wrote:
> -----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

Will Do. Change sw to SW

> ### examples/eventdev_pipeline: added sample app

Will Do. Add _sw_pmd

> 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

These are false positives. The text in the messages are not meant to be 
the function name.
If anything, I would prefer to change the function names to have " _thread"?

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

I will investigate and post a new patch in a few hours.

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

Sure. I'm using this:
./examples/eventdev_pipeline_sw_pmd/build/app/eventdev_pipeline_sw_pmd 
-c ff0  -w 0000:18:00.0 -w 0000:18:00.1 -w 0000:18:00.2 -w 0000:18:00.3 
--vdev=event_sw0 -- -s 4 -r 10 -t 10 -e 20 -w f00



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


More information about the dev mailing list