From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 782B0C438 for ; Thu, 28 Jan 2016 12:54:12 +0100 (CET) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id AD7D48E760; Thu, 28 Jan 2016 11:54:11 +0000 (UTC) Received: from sopuli.koti.laiskiainen.org (vpn1-4-63.ams2.redhat.com [10.36.4.63]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0SBs9fZ006279; Thu, 28 Jan 2016 06:54:10 -0500 To: Fan Zhang , 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> From: Panu Matilainen Message-ID: <56AA0161.30308@redhat.com> Date: Thu, 28 Jan 2016 13:54:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <1453916363-26709-2-git-send-email-roy.fan.zhang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 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: Thu, 28 Jan 2016 11:54:12 -0000 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? [...] > 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 -