From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 2A2BBB47D for ; Tue, 9 Feb 2016 11:53:12 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP; 09 Feb 2016 02:53:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,420,1449561600"; d="scan'208";a="649474436" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by FMSMGA003.fm.intel.com with ESMTP; 09 Feb 2016 02:53:09 -0800 Received: from sivlogin002.ir.intel.com (sivlogin002.ir.intel.com [10.237.217.37]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id u19Ar9Pv020378; Tue, 9 Feb 2016 10:53:09 GMT Received: from sivlogin002.ir.intel.com (localhost [127.0.0.1]) by sivlogin002.ir.intel.com with ESMTP id u19Ar9aq031255; Tue, 9 Feb 2016 10:53:09 GMT Received: (from fyigit@localhost) by sivlogin002.ir.intel.com with œ id u19Ar8kG031251; Tue, 9 Feb 2016 10:53:09 GMT X-Authentication-Warning: sivlogin002.ir.intel.com: fyigit set sender to ferruh.yigit@intel.com using -f Date: Tue, 9 Feb 2016 10:53:08 +0000 From: Ferruh Yigit To: Reshma Pattan Message-ID: <20160209105308.GA30820@sivlogin002.ir.intel.com> Mail-Followup-To: Reshma Pattan , dev@dpdk.org References: <1453912360-18179-1-git-send-email-ferruh.yigit@intel.com> <1453912360-18179-2-git-send-email-ferruh.yigit@intel.com> <56B8CD0E.5050104@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56B8CD0E.5050104@intel.com> User-Agent: Mutt/1.5.17 (2007-11-01) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 1/2] kdp: add kernel data path kernel module 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: Tue, 09 Feb 2016 10:53:12 -0000 On Mon, Feb 08, 2016 at 05:14:54PM +0000, Reshma Pattan wrote: Hi Reshma, > > > 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. > Thanks, when that patch applied, I can rebase 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? > Can't remove, this struct should match with rte_mbuf >> + 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? > This is kernel module. Doesn't know about userspace library macros. > >> 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? > I think extra "Debug" prefix is not required here. > >> 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 > This is private header. > >> 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? > Yes, this is part of net_device_ops, and required to fake multicast support. Regards, ferruh