From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 71E13235 for ; Thu, 6 Jul 2017 12:04:45 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jul 2017 03:04:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,316,1496127600"; d="scan'208";a="282837897" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.221.55]) ([10.237.221.55]) by fmsmga004.fm.intel.com with ESMTP; 06 Jul 2017 03:04:42 -0700 To: Jerin Jacob References: <1498751388-41571-2-git-send-email-david.hunt@intel.com> <1498830673-56759-1-git-send-email-david.hunt@intel.com> <1498830673-56759-2-git-send-email-david.hunt@intel.com> <20170703035755.GA6275@jerin> <25452a77-c5ae-97e8-b41a-5dfcb9fb19a6@intel.com> <20170705052853.GA8031@jerin> <20170706033120.GA10973@jerin> Cc: dev@dpdk.org, harry.van.haaren@intel.com, Gage Eads , Bruce Richardson From: "Hunt, David" Message-ID: <7987361f-935e-a689-103d-276fe3a4c36e@intel.com> Date: Thu, 6 Jul 2017 11:04:41 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170706033120.GA10973@jerin> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v5 1/3] examples/eventdev_pipeline: added sample app X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jul 2017 10:04:46 -0000 On 6/7/2017 4:31 AM, Jerin Jacob wrote: > -----Original Message----- >> Date: Wed, 5 Jul 2017 12:15:51 +0100 >> From: "Hunt, David" >> To: Jerin Jacob >> CC: dev@dpdk.org, harry.van.haaren@intel.com, Gage Eads >> , Bruce Richardson >> 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" >>>> To: Jerin Jacob >>>> CC: dev@dpdk.org, harry.van.haaren@intel.com, Gage Eads >>>> , Bruce Richardson >>>> 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(ð->d_addr, &addr); >>>>>> + ether_addr_copy(ð->s_addr, ð->d_addr); >>>>>> + ether_addr_copy(&addr, ð->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. >