From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 7A5F2C6B6 for ; Fri, 29 Jan 2016 16:10:45 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 29 Jan 2016 07:10:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,364,1449561600"; d="scan'208";a="871767173" Received: from fanzhan2-mobl.ger.corp.intel.com (HELO [10.237.221.58]) ([10.237.221.58]) by orsmga001.jf.intel.com with ESMTP; 29 Jan 2016 07:10:43 -0800 To: Panu Matilainen , dev@dpdk.org References: <1453916363-26709-1-git-send-email-roy.fan.zhang@intel.com> <1453916363-26709-2-git-send-email-roy.fan.zhang@intel.com> <56AA0161.30308@redhat.com> From: "Zhang, Roy Fan" Message-ID: <56AB80F2.2070609@intel.com> Date: Fri, 29 Jan 2016 15:10:42 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56AA0161.30308@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/4] lib/librte_port: add PCAP file support to source port X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Jan 2016 15:10:46 -0000 Hi Panu, Thank you for the careful review and comments. On 28/01/2016 11:54, Panu Matilainen wrote: > On 01/27/2016 07:39 PM, Fan Zhang wrote: >> Originally, source ports in librte_port is an input port used as packet >> generator. Similar to Linux kernel /dev/zero character device, it >> generates null packets. This patch adds optional PCAP file support to >> source port: instead of sending NULL packets, the source port generates >> packets copied from a PCAP file. To increase the performance, the >> packets >> in the file are loaded to memory initially, and copied to mbufs in >> circular >> manner. Users can enable or disable this feature by setting >> CONFIG_RTE_PORT_PCAP compiler option "y" or "n". >> >> Signed-off-by: Fan Zhang >> Acked-by: Cristian Dumitrescu >> --- >> config/common_bsdapp | 1 + >> config/common_linuxapp | 1 + >> lib/librte_port/Makefile | 4 + >> lib/librte_port/rte_port_source_sink.c | 190 >> +++++++++++++++++++++++++++++++++ >> lib/librte_port/rte_port_source_sink.h | 7 ++ >> mk/rte.app.mk | 1 + >> 6 files changed, 204 insertions(+) >> > [...] >> +#ifdef RTE_PORT_PCAP >> + >> +/** >> + * Load PCAP file, allocate and copy packets in the file to memory >> + * >> + * @param p >> + * Parameters for source port >> + * @param port >> + * Handle to source port >> + * @param socket_id >> + * Socket id where the memory is created >> + * @return >> + * 0 on SUCCESS >> + * error code otherwise >> + */ >> +static int >> +pcap_source_load(struct rte_port_source_params *p, >> + struct rte_port_source *port, >> + int socket_id) >> +{ > [...] >> +#else >> +static int >> +pcap_source_load(__rte_unused struct rte_port_source_params *p, >> + struct rte_port_source *port, >> + __rte_unused int socket_id) >> +{ >> + port->pkt_buff = NULL; >> + port->pkt_len = NULL; >> + port->pkts = NULL; >> + port->pkt_index = 0; >> + >> + return 0; >> +} >> +#endif > > Same as in patch 3/4, shouldn't this return -ENOTSUP when pcap support > is not built in, instead of success? Thank you for the suggestion. I will make the change in V2. > [...] > >> diff --git a/lib/librte_port/rte_port_source_sink.h >> b/lib/librte_port/rte_port_source_sink.h >> index 0f9be79..6f39bec 100644 >> --- a/lib/librte_port/rte_port_source_sink.h >> +++ b/lib/librte_port/rte_port_source_sink.h >> @@ -53,6 +53,13 @@ extern "C" { >> struct rte_port_source_params { >> /** Pre-initialized buffer pool */ >> struct rte_mempool *mempool; >> + /** The full path of the pcap file to read packets from */ >> + char *file_name; >> + /** The number of bytes to be read from each packet in the >> + * pcap file. If this value is 0, the whole packet is read; >> + * if it is bigger than packet size, the generated packets >> + * will contain the whole packet */ >> + uint32_t n_bytes_per_pkt; >> }; > > This is a likely ABI-break. It "only" appends to the struct, which > might in some cases be okay but only when there's no sensible use for > the struct within arrays or embedded in structs. The ip_pipeline > example for example embeds struct rte_port_source_params within > another struct which is could be thought of as an indication that > other applications might be doing this as well. > > An ABI break for librte_port has not been announced for 2.3 so you'd > need to announce the intent to do so in 2.4 now, and then either wait > till post 2.3 or wrap this in CONFIG_RTE_NEXT_ABI. > - Panu - Before Submitting the patch I have run validate-abi script, the validation results showed "compatible" on all libraries. Also, the patch's added line in the rte_port_source_sink.c was ensured that, if the CONFIG_RTE_PORT_PCAP compiler option is set to "n" (by default), the added read-only elements in the new rte_source_params structure won't be touched. CONFIG_RTE_PORT_PCAP compiler option lies in config/comm_bsdapp and config/com_linuxapp, and is freshly added in this patch. If an application is built on top of latest release version of rte_port_source_sink library, it shall work fine with the new library if the new CONFIG_RTE_PORT_PCAP opition is left the default "n". ip_pipeline example works fine this way. I hope this changes your mind on breaking the ABI. Best regards, Fan