From mboxrd@z Thu Jan 1 00:00:00 1970 From: Reshma Pattan Subject: Re: [PATCH 1/2] kdp: add kernel data path kernel module Date: Mon, 8 Feb 2016 17:14:54 +0000 Message-ID: <56B8CD0E.5050104@intel.com> References: <1453912360-18179-1-git-send-email-ferruh.yigit@intel.com> <1453912360-18179-2-git-send-email-ferruh.yigit@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: Ferruh Yigit , dev@dpdk.org Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 53BF55677 for ; Mon, 8 Feb 2016 18:15:00 +0100 (CET) In-Reply-To: <1453912360-18179-2-git-send-email-ferruh.yigit@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/27/2016 4:32 PM, Ferruh Yigit wrote: > This kernel module is based on KNI module, but this one is stripped > version of it and only for data messages, no control functionality > provided. > > FIFO implementation of the KNI is kept exact same, but ethtool related > code removed and virtual network management related code simplified. > > This module contains kernel support to create network devices and > this module has a simple driver for virtual network device, the driver > simply puts/gets packets to/from FIFO instead of real hardware. > > FIFO is created owned by userspace application, which is for this case > KDP PMD. > > In long term this patch intends to replace the KNI and KNI will be > depreciated. > > Signed-off-by: Ferruh Yigit > --- > > > diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kdp_common.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kdp_common.h > new file mode 100644 > index 0000000..0c77f58 > --- /dev/null > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kdp_common.h > > +/** > + * KDP name is part of memzone name. > + */ > +#define RTE_KDP_NAMESIZE 32 > + > +#ifndef RTE_CACHE_LINE_SIZE > +#define RTE_CACHE_LINE_SIZE 64 /**< Cache line size. */ > +#endif Jerin Jacob has patch for cleaning of MACRO RTE_CACHE_LINE_SIZE and having CONFIG_RTE_CACHE_LINE_SIZE in config file. You may need to remove this,once those changes are available in code. > + > +/* > + * The kernel image of the rte_mbuf struct, with only the relevant fields. > + * Padding is necessary to assure the offsets of these fields > + */ > +struct rte_kdp_mbuf { > + void *buf_addr __attribute__((__aligned__(RTE_CACHE_LINE_SIZE))); > + char pad0[10]; > + > + /**< Start address of data in segment buffer. */ > + uint16_t data_off; > + char pad1[4]; > + uint64_t ol_flags; /**< Offload features. */ You are not using ol_flags down in the code. Should this be removed? > + char pad2[4]; > + > + /**< Total pkt len: sum of all segment data_len. */ > + uint32_t pkt_len; > + > + /**< Amount of data in segment buffer. */ > + uint16_t data_len; > + > + /* fields on second cache line */ > + char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_SIZE))); > + void *pool; > + void *next; > +}; > + Does all structures should have "__rte_cache_aligned" in their declarations? Like other DPDK structs? > diff --git a/lib/librte_eal/linuxapp/kdp/kdp_dev.h b/lib/librte_eal/linuxapp/kdp/kdp_dev.h > new file mode 100644 > index 0000000..52952b4 > --- /dev/null > +++ b/lib/librte_eal/linuxapp/kdp/kdp_dev.h > > + > +#define KDP_ERR(args...) printk(KERN_DEBUG "KDP: Error: " args) > +#define KDP_PRINT(args...) printk(KERN_DEBUG "KDP: " args) > + > +#ifdef RTE_KDP_KO_DEBUG > +#define KDP_DBG(args...) printk(KERN_DEBUG "KDP: " args) Is it good to haveKERN_DEBUG "KDP:Debug: " like Errors? > diff --git a/lib/librte_eal/linuxapp/kdp/kdp_fifo.h b/lib/librte_eal/linuxapp/kdp/kdp_fifo.h > new file mode 100644 > index 0000000..a5fe080 > --- /dev/null > +++ b/lib/librte_eal/linuxapp/kdp/kdp_fifo.h > > +/** > + * Adds num elements into the fifo. Return the number actually written > + */ > +static inline unsigned > +kdp_fifo_put(struct rte_kdp_fifo *fifo, void **data, unsigned num) > +{ > + unsigned i = 0; > + unsigned fifo_write = fifo->write; > + unsigned fifo_read = fifo->read; > + unsigned new_write = fifo_write; > + > + for (i = 0; i < num; i++) { > + new_write = (new_write + 1) & (fifo->len - 1); > + > + if (new_write == fifo_read) > + break; > + fifo->buffer[fifo_write] = data[i]; > + fifo_write = new_write; > + } > + fifo->write = fifo_write; > + > + return i; > +} you can add header for all function declarations inside header file with below format. Same for other header files and functions. *@Description *@params *@Return value > diff --git a/lib/librte_eal/linuxapp/kdp/kdp_misc.c b/lib/librte_eal/linuxapp/kdp/kdp_misc.c > new file mode 100644 > index 0000000..d97d1c0 > --- /dev/null > +++ b/lib/librte_eal/linuxapp/kdp/kdp_misc.c > +static int > +kdp_compat_ioctl(struct inode *inode, unsigned int ioctl_num, > + unsigned long ioctl_param) > +{ > + /* 32 bits app on 64 bits OS to be supported later */ > + KDP_PRINT("Not implemented.\n"); Should this be warning/ERR instead of PRINT? > diff --git a/lib/librte_eal/linuxapp/kdp/kdp_net.c b/lib/librte_eal/linuxapp/kdp/kdp_net.c > new file mode 100644 > index 0000000..5c669f5 > --- /dev/null > +++ b/lib/librte_eal/linuxapp/kdp/kdp_net.c > + > +static void > +kdp_net_set_rx_mode(struct net_device *dev) > +{ > +} Empty function body? Thanks, Reshma