[dpdk-dev] [PATCH v4] KNI: use a memzone pool for KNI alloc/release

[dpdk-dev] [PATCH v4] KNI: use a memzone pool for KNI alloc/release

[Marc Sune]

This patch implements the KNI memzone pool in order to prevent
memzone exhaustion when allocating/deallocating KNI interfaces.

It adds a new API call, rte_kni_init(max_kni_ifaces) that shall
be called before any call to rte_kni_alloc() if KNI is used.

v2: Moved KNI fd opening to rte_kni_init(). Revised style.
v3: Adapted kni examples/tests to rte_kni_init().
v4: Improved example integration. Fixed kni_memzone_pool_alloc/release() bug.

Signed-off-by: Marc Sune <marc.sune@bisdn.de>
---
 app/test/test_kni.c      |    5 +-
 examples/kni/main.c      |   22 ++++
 lib/librte_kni/rte_kni.c |  317 +++++++++++++++++++++++++++++++++++++---------
 lib/librte_kni/rte_kni.h |   18 +++
 4 files changed, 302 insertions(+), 60 deletions(-)

diff --git a/app/test/test_kni.c b/app/test/test_kni.c
index 1081131..608901d 100644
--- a/app/test/test_kni.c
+++ b/app/test/test_kni.c
@@ -58,7 +58,7 @@
 
 #define IFCONFIG      "/sbin/ifconfig "
 #define TEST_KNI_PORT "test_kni_port"
-
+#define KNI_TEST_MAX_PORTS 4
 /* The threshold number of mbufs to be transmitted or received. */
 #define KNI_NUM_MBUF_THRESHOLD 100
 static int kni_pkt_mtu = 0;
@@ -498,6 +498,9 @@ test_kni(void)
 	struct rte_eth_dev_info info;
 	struct rte_kni_ops ops;
 
+	/* Initialize KNI subsytem */
+	rte_kni_init(KNI_TEST_MAX_PORTS);
+
 	if (test_kni_allocate_lcores() < 0) {
 		printf("No enough lcores for kni processing\n");
 		return -1;
diff --git a/examples/kni/main.c b/examples/kni/main.c
index 69d1ef2..acccd64 100644
--- a/examples/kni/main.c
+++ b/examples/kni/main.c
@@ -616,6 +616,25 @@ parse_args(int argc, char **argv)
 	return ret;
 }
 
+/* Initialize KNI subsystem */
+static void
+init_kni(void)
+{
+	unsigned int num_of_kni_ports = 0, i;
+	struct kni_port_params **params = kni_port_params_array;
+
+	/* Calculate the maximum number of KNI interfaces that will be used */
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (kni_port_params_array[i]) {
+			num_of_kni_ports += (params[i]->nb_lcore_k ?
+				params[i]->nb_lcore_k : 1);
+		}
+	}
+	
+	/* Invoke rte KNI init to preallocate the ports */	
+	rte_kni_init(num_of_kni_ports);
+}
+
 /* Initialise a single port on an Ethernet device */
 static void
 init_port(uint8_t port)
@@ -901,6 +920,9 @@ main(int argc, char** argv)
 		if (kni_port_params_array[i] && i >= nb_sys_ports)
 			rte_exit(EXIT_FAILURE, "Configured invalid "
 						"port ID %u\n", i);
+	
+	/* Initialize KNI subsystem */
+	init_kni();
 
 	/* Initialise each port */
 	for (port = 0; port < nb_sys_ports; port++) {
diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index 76feef4..4c7f099 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -40,6 +40,7 @@
 #include <unistd.h>
 #include <sys/ioctl.h>
 
+#include <rte_spinlock.h>
 #include <rte_string_fns.h>
 #include <rte_ethdev.h>
 #include <rte_malloc.h>
@@ -58,7 +59,7 @@
 
 #define KNI_REQUEST_MBUF_NUM_MAX      32
 
-#define KNI_MZ_CHECK(mz) do { if (mz) goto fail; } while (0)
+#define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while (0)
 
 /**
  * KNI context
@@ -66,6 +67,7 @@
 struct rte_kni {
 	char name[RTE_KNI_NAMESIZE];        /**< KNI interface name */
 	uint16_t group_id;                  /**< Group ID of KNI devices */
+	uint32_t slot_id;                   /**< KNI pool slot ID */
 	struct rte_mempool *pktmbuf_pool;   /**< pkt mbuf mempool */
 	unsigned mbuf_size;                 /**< mbuf size */
 
@@ -88,10 +90,48 @@ enum kni_ops_status {
 	KNI_REQ_REGISTERED,
 };
 
+/**
+ * KNI memzone pool slot
+ */
+struct rte_kni_memzone_slot{
+	uint32_t id;
+	uint8_t in_use : 1;                    /**< slot in use */
+
+	/* Memzones */
+	const struct rte_memzone *m_ctx;       /**< KNI ctx */
+	const struct rte_memzone *m_tx_q;      /**< TX queue */
+	const struct rte_memzone *m_rx_q;      /**< RX queue */
+	const struct rte_memzone *m_alloc_q;   /**< Allocated mbufs queue */
+	const struct rte_memzone *m_free_q;    /**< To be freed mbufs queue */
+	const struct rte_memzone *m_req_q;     /**< Request queue */
+	const struct rte_memzone *m_resp_q;    /**< Response queue */
+	const struct rte_memzone *m_sync_addr;         
+	
+	/* Free linked list */
+	struct rte_kni_memzone_slot *next;     /**< Next slot link.list */
+};
+
+/**
+ * KNI memzone pool
+ */
+struct rte_kni_memzone_pool{
+	uint8_t initialized : 1;            /**< Global KNI pool init flag */
+ 
+	uint32_t max_ifaces;                /**< Max. num of KNI ifaces */
+	struct rte_kni_memzone_slot *slots;        /**< Pool slots */
+	rte_spinlock_t mutex;               /**< alloc/relase mutex */
+
+	/* Free memzone slots linked-list */
+	struct rte_kni_memzone_slot *free;         /**< First empty slot */
+	struct rte_kni_memzone_slot *free_tail;    /**< Last empty slot */
+};
+
+
 static void kni_free_mbufs(struct rte_kni *kni);
 static void kni_allocate_mbufs(struct rte_kni *kni);
 
 static volatile int kni_fd = -1;
+static struct rte_kni_memzone_pool kni_memzone_pool = {0};
 
 static const struct rte_memzone *
 kni_memzone_reserve(const char *name, size_t len, int socket_id,
@@ -105,6 +145,163 @@ kni_memzone_reserve(const char *name, size_t len, int socket_id,
 	return mz;
 }
 
+/* Pool mgmt */
+static struct rte_kni_memzone_slot*
+kni_memzone_pool_alloc(void)
+{
+	struct rte_kni_memzone_slot* slot;
+	
+	rte_spinlock_lock(&kni_memzone_pool.mutex);	
+
+	if(!kni_memzone_pool.free) {
+		rte_spinlock_unlock(&kni_memzone_pool.mutex);	
+		return NULL;
+	}
+
+	slot = kni_memzone_pool.free;
+	kni_memzone_pool.free = slot->next;
+	slot->in_use = 1;
+
+	if(!kni_memzone_pool.free)
+		kni_memzone_pool.free_tail = NULL;
+
+	rte_spinlock_unlock(&kni_memzone_pool.mutex);
+
+	return slot;
+}
+
+static void 
+kni_memzone_pool_release(struct rte_kni_memzone_slot* slot)
+{
+	rte_spinlock_lock(&kni_memzone_pool.mutex);	
+
+	if(kni_memzone_pool.free)
+		kni_memzone_pool.free_tail->next = slot;
+	else
+		kni_memzone_pool.free = slot;
+
+	kni_memzone_pool.free_tail = slot;
+	slot->next = NULL;
+	slot->in_use = 0;
+
+	rte_spinlock_unlock(&kni_memzone_pool.mutex);
+}
+
+
+/* Shall be called before any allocation happens */
+void
+rte_kni_init(unsigned int max_kni_ifaces)
+{
+	uint32_t i;
+	struct rte_kni_memzone_slot* it;
+	const struct rte_memzone *mz;
+#define OBJNAMSIZ 32
+	char obj_name[OBJNAMSIZ];
+	char mz_name[RTE_MEMZONE_NAMESIZE];
+
+	if(max_kni_ifaces == 0) {
+		RTE_LOG(ERR, KNI, "Invalid number of max_kni_ifaces %d\n",
+							max_kni_ifaces);
+		rte_panic("Unable to initialize KNI\n");
+	}
+
+	/* Check FD and open */
+	if (kni_fd < 0) {
+		kni_fd = open("/dev/" KNI_DEVICE, O_RDWR);
+		if (kni_fd < 0) {
+			rte_panic("Can not open /dev/%s\n", KNI_DEVICE);
+		}
+	}
+
+	/* Allocate slot objects */
+	kni_memzone_pool.slots = (struct rte_kni_memzone_slot*)rte_malloc(NULL,
+					sizeof(struct rte_kni_memzone_slot) *
+					max_kni_ifaces,
+					0);
+	KNI_MEM_CHECK(kni_memzone_pool.slots == NULL);
+
+	/* Initialize general pool variables */
+	kni_memzone_pool.initialized = 1;
+	kni_memzone_pool.max_ifaces = max_kni_ifaces;
+	kni_memzone_pool.free = &kni_memzone_pool.slots[0];
+
+	/* Pre-allocate all memzones of all the slots; panic on error */
+	for(i=0; i<max_kni_ifaces; i++) {
+
+		/* Recover current slot */
+		it = &kni_memzone_pool.slots[i];
+		it->id = i;
+	
+		/* Allocate KNI context */
+		snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "KNI_INFO_%d", i);
+		mz = kni_memzone_reserve(mz_name, sizeof(struct rte_kni),
+					SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_ctx = mz;
+			
+		/* TX RING */
+		snprintf(obj_name, OBJNAMSIZ, "kni_tx_%d", i);
+		mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, 
+							SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_tx_q = mz;
+
+		/* RX RING */
+		snprintf(obj_name, OBJNAMSIZ, "kni_rx_%d", i);
+		mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, 
+							SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_rx_q = mz;
+
+		/* ALLOC RING */
+		snprintf(obj_name, OBJNAMSIZ, "kni_alloc_%d", i);
+		mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE,
+							SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_alloc_q = mz;
+
+		/* FREE RING */
+		snprintf(obj_name, OBJNAMSIZ, "kni_free_%d", i);
+		mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, 
+							SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_free_q = mz;
+
+		/* Request RING */
+		snprintf(obj_name, OBJNAMSIZ, "kni_req_%d", i);
+		mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, 
+							SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_req_q = mz;
+
+		/* Response RING */
+		snprintf(obj_name, OBJNAMSIZ, "kni_resp_%d", i);
+		mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, 
+							SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_resp_q = mz;
+
+		/* Req/Resp sync mem area */
+		snprintf(obj_name, OBJNAMSIZ, "kni_sync_%d", i);
+		mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, 
+							SOCKET_ID_ANY, 0);
+		KNI_MEM_CHECK(mz == NULL);
+		it->m_sync_addr = mz;
+
+		if(i+1 == max_kni_ifaces) {
+			it->next = NULL;
+			kni_memzone_pool.free_tail = it;
+		}else
+			it->next = &kni_memzone_pool.slots[i+1];
+	}
+	
+	return;
+
+kni_fail:
+	rte_panic("Unable to allocate memory for max_kni_ifaces:%d."
+		"increase the amount of hugepages memory\n", max_kni_ifaces);
+}
+
 /* It is deprecated and just for backward compatibility */
 struct rte_kni *
 rte_kni_create(uint8_t port_id,
@@ -140,34 +337,37 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
 	struct rte_kni_device_info dev_info;
 	struct rte_kni *ctx;
 	char intf_name[RTE_KNI_NAMESIZE];
-#define OBJNAMSIZ 32
-	char obj_name[OBJNAMSIZ];
 	char mz_name[RTE_MEMZONE_NAMESIZE];
 	const struct rte_memzone *mz;
+	struct rte_kni_memzone_slot* slot=NULL;
 
 	if (!pktmbuf_pool || !conf || !conf->name[0])
 		return NULL;
 
-	/* Check FD and open once */
-	if (kni_fd < 0) {
-		kni_fd = open("/dev/" KNI_DEVICE, O_RDWR);
-		if (kni_fd < 0) {
-			RTE_LOG(ERR, KNI, "Can not open /dev/%s\n",
-							KNI_DEVICE);
-			return NULL;
-		}
+	/* Check if KNI subsystem has been initialized */
+	if (kni_memzone_pool.initialized != 1) {
+		RTE_LOG(ERR, KNI, "KNI subsystem has not been initialized. "
+				"Invoke rte_kni_init() first\n");
+		return NULL;
 	}
 
+	/* Get an available slot from the pool */
+	slot = kni_memzone_pool_alloc();
+	if(!slot) {
+		RTE_LOG(ERR, KNI, "Cannot allocate more KNI interfaces; "
+			"increase the number of max_kni_ifaces(current %d) or "
+			"release unusued ones.\n", 
+			kni_memzone_pool.max_ifaces);
+		return NULL;
+	}
+	
+	/* Recover ctx */
+	ctx = slot->m_ctx->addr;
 	snprintf(intf_name, RTE_KNI_NAMESIZE, "%s", conf->name);
-	snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "KNI_INFO_%s", intf_name);
-	mz = kni_memzone_reserve(mz_name, sizeof(struct rte_kni),
-				SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
-	ctx = mz->addr;
 
 	if (ctx->in_use) {
 		RTE_LOG(ERR, KNI, "KNI %s is in use\n", ctx->name);
-		goto fail;
+		return NULL;
 	}
 	memset(ctx, 0, sizeof(struct rte_kni));
 	if (ops)
@@ -190,83 +390,72 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
 	RTE_LOG(INFO, KNI, "pci: %02x:%02x:%02x \t %02x:%02x\n",
 		dev_info.bus, dev_info.devid, dev_info.function,
 			dev_info.vendor_id, dev_info.device_id);
-
 	/* TX RING */
-	snprintf(obj_name, OBJNAMSIZ, "kni_tx_%s", intf_name);
-	mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
+	mz = slot->m_tx_q;
 	ctx->tx_q = mz->addr;
 	kni_fifo_init(ctx->tx_q, KNI_FIFO_COUNT_MAX);
 	dev_info.tx_phys = mz->phys_addr;
 
 	/* RX RING */
-	snprintf(obj_name, OBJNAMSIZ, "kni_rx_%s", intf_name);
-	mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
+	mz = slot->m_rx_q;
 	ctx->rx_q = mz->addr;
 	kni_fifo_init(ctx->rx_q, KNI_FIFO_COUNT_MAX);
 	dev_info.rx_phys = mz->phys_addr;
 
 	/* ALLOC RING */
-	snprintf(obj_name, OBJNAMSIZ, "kni_alloc_%s", intf_name);
-	mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
+	mz = slot->m_alloc_q;
 	ctx->alloc_q = mz->addr;
 	kni_fifo_init(ctx->alloc_q, KNI_FIFO_COUNT_MAX);
 	dev_info.alloc_phys = mz->phys_addr;
 
 	/* FREE RING */
-	snprintf(obj_name, OBJNAMSIZ, "kni_free_%s", intf_name);
-	mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
+	mz = slot->m_free_q;
 	ctx->free_q = mz->addr;
 	kni_fifo_init(ctx->free_q, KNI_FIFO_COUNT_MAX);
 	dev_info.free_phys = mz->phys_addr;
 
 	/* Request RING */
-	snprintf(obj_name, OBJNAMSIZ, "kni_req_%s", intf_name);
-	mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
+	mz = slot->m_req_q;
 	ctx->req_q = mz->addr;
 	kni_fifo_init(ctx->req_q, KNI_FIFO_COUNT_MAX);
 	dev_info.req_phys = mz->phys_addr;
 
 	/* Response RING */
-	snprintf(obj_name, OBJNAMSIZ, "kni_resp_%s", intf_name);
-	mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
+	mz = slot->m_resp_q;
 	ctx->resp_q = mz->addr;
 	kni_fifo_init(ctx->resp_q, KNI_FIFO_COUNT_MAX);
 	dev_info.resp_phys = mz->phys_addr;
 
 	/* Req/Resp sync mem area */
-	snprintf(obj_name, OBJNAMSIZ, "kni_sync_%s", intf_name);
-	mz = kni_memzone_reserve(obj_name, KNI_FIFO_SIZE, SOCKET_ID_ANY, 0);
-	KNI_MZ_CHECK(mz == NULL);
+	mz = slot->m_sync_addr; 
 	ctx->sync_addr = mz->addr;
 	dev_info.sync_va = mz->addr;
 	dev_info.sync_phys = mz->phys_addr;
 
+
 	/* MBUF mempool */
 	snprintf(mz_name, sizeof(mz_name), RTE_MEMPOOL_OBJ_NAME,
 		pktmbuf_pool->name);
 	mz = rte_memzone_lookup(mz_name);
-	KNI_MZ_CHECK(mz == NULL);
+	KNI_MEM_CHECK(mz == NULL);
 	dev_info.mbuf_va = mz->addr;
 	dev_info.mbuf_phys = mz->phys_addr;
 	ctx->pktmbuf_pool = pktmbuf_pool;
 	ctx->group_id = conf->group_id;
+	ctx->slot_id = slot->id;
 	ctx->mbuf_size = conf->mbuf_size;
 
 	ret = ioctl(kni_fd, RTE_KNI_IOCTL_CREATE, &dev_info);
-	KNI_MZ_CHECK(ret < 0);
+	KNI_MEM_CHECK(ret < 0);
 
 	ctx->in_use = 1;
 
 	return ctx;
 
-fail:
-
+kni_fail:
+	if(slot)
+		kni_memzone_pool_release(&kni_memzone_pool.slots[slot->id]);
+			
 	return NULL;
 }
 
@@ -287,6 +476,7 @@ int
 rte_kni_release(struct rte_kni *kni)
 {
 	struct rte_kni_device_info dev_info;
+	uint32_t slot_id;
 
 	if (!kni || !kni->in_use)
 		return -1;
@@ -302,8 +492,19 @@ rte_kni_release(struct rte_kni *kni)
 	kni_free_fifo(kni->rx_q);
 	kni_free_fifo(kni->alloc_q);
 	kni_free_fifo(kni->free_q);
+
+	slot_id = kni->slot_id;		
+
+	/* Memset the KNI struct */
 	memset(kni, 0, sizeof(struct rte_kni));
 
+	/* Release memzone */
+	if(slot_id > kni_memzone_pool.max_ifaces) {
+		rte_panic("KNI pool: corrupted slot ID: %d, max: %d\n", 
+			slot_id, kni_memzone_pool.max_ifaces);
+	}
+	kni_memzone_pool_release(&kni_memzone_pool.slots[slot_id]);
+
 	return 0;
 }
 
@@ -437,23 +638,21 @@ rte_kni_get_port_id(struct rte_kni *kni)
 struct rte_kni *
 rte_kni_get(const char *name)
 {
-	struct rte_kni *kni;
-	const struct rte_memzone *mz;
-	char mz_name[RTE_MEMZONE_NAMESIZE];
-
-	if (!name || !name[0])
-		return NULL;
-
-	snprintf(mz_name, RTE_MEMZONE_NAMESIZE, "KNI_INFO_%s", name);
-	mz = rte_memzone_lookup(mz_name);
-	if (!mz)
-		return NULL;
-
-	kni = mz->addr;
-	if (!kni->in_use)
-		return NULL;
+	uint32_t i;
+	struct rte_kni_memzone_slot* it;
+	struct rte_kni* kni;
+
+	/* Note: could be improved perf-wise if necessary */
+	for(i=0; i<kni_memzone_pool.max_ifaces; i++) {
+		it = &kni_memzone_pool.slots[i];
+		if(it->in_use == 0)
+			continue;
+		kni = it->m_ctx->addr;
+		if(strncmp(kni->name, name, RTE_KNI_NAMESIZE) == 0)
+			return kni;
+	}
 
-	return kni;
+	return NULL;
 }
 
 /*
diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h
index 1a0b004..0159a1d 100644
--- a/lib/librte_kni/rte_kni.h
+++ b/lib/librte_kni/rte_kni.h
@@ -90,11 +90,27 @@ struct rte_kni_conf {
 };
 
 /**
+ * Initialize and preallocate KNI subsystem
+ *
+ * This function is to be executed on the MASTER lcore only, after EAL 
+ * initialization and before any KNI interface is attempted to be
+ * allocated
+ * 
+ * @param max_kni_ifaces 
+ *  The maximum number of KNI interfaces that can coexist concurrently
+ */
+extern void rte_kni_init(unsigned int max_kni_ifaces);
+
+
+/**
  * Allocate KNI interface according to the port id, mbuf size, mbuf pool,
  * configurations and callbacks for kernel requests.The KNI interface created
  * in the kernel space is the net interface the traditional Linux application
  * talking to.
  *
+ * The rte_kni_alloc shall not be called before rte_kni_init() has been
+ * called. rte_kni_alloc is thread safe. 
+ *
  * @param pktmbuf_pool
  *  The mempool for allocting mbufs for packets.
  * @param conf
@@ -138,6 +154,8 @@ extern struct rte_kni *rte_kni_create(uint8_t port_id,
  * Release KNI interface according to the context. It will also release the
  * paired KNI interface in kernel space. All processing on the specific KNI
  * context need to be stopped before calling this interface.
+ * 
+ * rte_kni_release is thread safe.
  *
  * @param kni
  *  The pointer to the context of an existent KNI interface.
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH v4] KNI: use a memzone pool for KNI alloc/release

[Zhang, Helin]

> This patch implements the KNI memzone pool in order to prevent memzone
> exhaustion when allocating/deallocating KNI interfaces.
> 
> It adds a new API call, rte_kni_init(max_kni_ifaces) that shall be called before
> any call to rte_kni_alloc() if KNI is used.
> 
> v2: Moved KNI fd opening to rte_kni_init(). Revised style.
> v3: Adapted kni examples/tests to rte_kni_init().
> v4: Improved example integration. Fixed kni_memzone_pool_alloc/release()
> bug.
> 
> Signed-off-by: Marc Sune <marc.sune@bisdn.de>

Acked-by: Helin Zhang <helin.zhang@intel.com>

> ---
>  app/test/test_kni.c      |    5 +-
>  examples/kni/main.c      |   22 ++++
>  lib/librte_kni/rte_kni.c |  317
> +++++++++++++++++++++++++++++++++++++---------
>  lib/librte_kni/rte_kni.h |   18 +++
>  4 files changed, 302 insertions(+), 60 deletions(-)

Re: [dpdk-dev] [PATCH v4] KNI: use a memzone pool for KNI alloc/release

[Thomas Monjalon]

Hi Marc,

2014-10-18 00:51, Marc Sune:
> This patch implements the KNI memzone pool in order to prevent
> memzone exhaustion when allocating/deallocating KNI interfaces.
> 
> It adds a new API call, rte_kni_init(max_kni_ifaces) that shall
> be called before any call to rte_kni_alloc() if KNI is used.
> 
> v2: Moved KNI fd opening to rte_kni_init(). Revised style.
> v3: Adapted kni examples/tests to rte_kni_init().
> v4: Improved example integration. Fixed kni_memzone_pool_alloc/release() bug.
> 
> Signed-off-by: Marc Sune <marc.sune@bisdn.de>

Thanks for the good work with Helin.
Before applying this patch, I'd like another version explaining in the
commit log why this change is needed.
And please use to checkpatch.pl to check and remove whitespace errors.

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH v4] KNI: use a memzone pool for KNI alloc/release

[Marc Sune]

Thomas,

v5: commit message arranged, all warnings from checkpatch.pl fixed except:

WARNING: Macros with flow control statements should be avoided
#104: FILE: lib/librte_kni/rte_kni.c:62:
+#define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while (0)

a) This MACRO was there before, I just re-factored it to make it more 
readable.
b) There are 4 lines exceeding 80cols due to long quoted strings. I 
followed kernel convention not to split them in multiple lines.

Thanks and regards
Marc

On 21/10/14 10:29, Thomas Monjalon wrote:
> Hi Marc,
>
> 2014-10-18 00:51, Marc Sune:
>> This patch implements the KNI memzone pool in order to prevent
>> memzone exhaustion when allocating/deallocating KNI interfaces.
>>
>> It adds a new API call, rte_kni_init(max_kni_ifaces) that shall
>> be called before any call to rte_kni_alloc() if KNI is used.
>>
>> v2: Moved KNI fd opening to rte_kni_init(). Revised style.
>> v3: Adapted kni examples/tests to rte_kni_init().
>> v4: Improved example integration. Fixed kni_memzone_pool_alloc/release() bug.
>>
>> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
> Thanks for the good work with Helin.
> Before applying this patch, I'd like another version explaining in the
> commit log why this change is needed.
> And please use to checkpatch.pl to check and remove whitespace errors.
>
> Thanks

Re: [dpdk-dev] [PATCH v4] KNI: use a memzone pool for KNI alloc/release

[Liu, Jijiang]

There is a compilation error using gcc version 4.6.2.

./lib/librte_kni/rte_kni.c:134:15: error: missing initializer [-Werror=missing-field-initializers]
/lib/librte_kni/rte_kni.c:134:15: error: (near initialization for kni_memzone_pool.max_iface? [-Werror=missing-field-initializers]


cc1: all warnings being treated as errors
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marc Sune
> Sent: Tuesday, October 21, 2014 6:52 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4] KNI: use a memzone pool for KNI
> alloc/release
> 
> Thomas,
> 
> v5: commit message arranged, all warnings from checkpatch.pl fixed except:
> 
> WARNING: Macros with flow control statements should be avoided
> #104: FILE: lib/librte_kni/rte_kni.c:62:
> +#define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while (0)
> 
> a) This MACRO was there before, I just re-factored it to make it more
> readable.
> b) There are 4 lines exceeding 80cols due to long quoted strings. I followed
> kernel convention not to split them in multiple lines.
> 
> Thanks and regards
> Marc
> 
> On 21/10/14 10:29, Thomas Monjalon wrote:
> > Hi Marc,
> >
> > 2014-10-18 00:51, Marc Sune:
> >> This patch implements the KNI memzone pool in order to prevent
> >> memzone exhaustion when allocating/deallocating KNI interfaces.
> >>
> >> It adds a new API call, rte_kni_init(max_kni_ifaces) that shall be
> >> called before any call to rte_kni_alloc() if KNI is used.
> >>
> >> v2: Moved KNI fd opening to rte_kni_init(). Revised style.
> >> v3: Adapted kni examples/tests to rte_kni_init().
> >> v4: Improved example integration. Fixed
> kni_memzone_pool_alloc/release() bug.
> >>
> >> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
> > Thanks for the good work with Helin.
> > Before applying this patch, I'd like another version explaining in the
> > commit log why this change is needed.
> > And please use to checkpatch.pl to check and remove whitespace errors.
> >
> > Thanks

[dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Marc Sune]

Fix for compilation warning 'missing-field-initializers' for some
GCC and clang versions introduced in commit 0c6bc8e

Signed-off-by: Marc Sune <marc.sune@bisdn.de>
---
 lib/librte_kni/rte_kni.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index f64a0a8..de29b99 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -131,7 +131,14 @@ static void kni_free_mbufs(struct rte_kni *kni);
 static void kni_allocate_mbufs(struct rte_kni *kni);
 
 static volatile int kni_fd = -1;
-static struct rte_kni_memzone_pool kni_memzone_pool = {0};
+static struct rte_kni_memzone_pool kni_memzone_pool = {
+	.initialized = 0,
+	.max_ifaces = 0,
+	.slots = 0,
+	.mutex =  RTE_SPINLOCK_INITIALIZER,
+	.free = NULL,
+	.free_tail = NULL
+};
 
 static const struct rte_memzone *
 kni_memzone_reserve(const char *name, size_t len, int socket_id,
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Marc Sune]

Liu,

Can you confirm that this patch fixes the issue?

Thanks
marc

On 22/10/14 09:10, Marc Sune wrote:
> Fix for compilation warning 'missing-field-initializers' for some
> GCC and clang versions introduced in commit 0c6bc8e
>
> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
> ---
>   lib/librte_kni/rte_kni.c |    9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index f64a0a8..de29b99 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -131,7 +131,14 @@ static void kni_free_mbufs(struct rte_kni *kni);
>   static void kni_allocate_mbufs(struct rte_kni *kni);
>   
>   static volatile int kni_fd = -1;
> -static struct rte_kni_memzone_pool kni_memzone_pool = {0};
> +static struct rte_kni_memzone_pool kni_memzone_pool = {
> +	.initialized = 0,
> +	.max_ifaces = 0,
> +	.slots = 0,
> +	.mutex =  RTE_SPINLOCK_INITIALIZER,
> +	.free = NULL,
> +	.free_tail = NULL
> +};
>   
>   static const struct rte_memzone *
>   kni_memzone_reserve(const char *name, size_t len, int socket_id,

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Liu, Jijiang]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marc Sune
> Sent: Wednesday, October 22, 2014 3:11 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-
> initializers'
> 
> Fix for compilation warning 'missing-field-initializers' for some GCC and clang
> versions introduced in commit 0c6bc8e
> 
> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
> ---
>  lib/librte_kni/rte_kni.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c index
> f64a0a8..de29b99 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -131,7 +131,14 @@ static void kni_free_mbufs(struct rte_kni *kni);
> static void kni_allocate_mbufs(struct rte_kni *kni);
> 
>  static volatile int kni_fd = -1;
> -static struct rte_kni_memzone_pool kni_memzone_pool = {0};
> +static struct rte_kni_memzone_pool kni_memzone_pool = {
> +	.initialized = 0,
> +	.max_ifaces = 0,
> +	.slots = 0,
> +	.mutex =  RTE_SPINLOCK_INITIALIZER,
> +	.free = NULL,
> +	.free_tail = NULL
> +};
> 
>  static const struct rte_memzone *
>  kni_memzone_reserve(const char *name, size_t len, int socket_id,
> --
> 1.7.10.4

Acked-by: Jijiang Liu<Jijiang.liu@intel.com>

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Thomas Monjalon]

2014-10-22 09:10, Marc Sune:
> Fix for compilation warning 'missing-field-initializers' for some
> GCC and clang versions introduced in commit 0c6bc8e
> 
> Signed-off-by: Marc Sune <marc.sune@bisdn.de>

It's not needed to initialize all fields.
This should be sufficient:
+static struct rte_kni_memzone_pool kni_memzone_pool = {.initialized = 0};

-- 
Thomas

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Marc Sune]

The mutex needs to be initialized to RTE_SPINLOCK_INITIALIZER(0) too, or 
move the initialization of the mutex to rte_kni_init().

I can prepare a second patch with one or the other option, if you want.

marc

On 22/10/14 10:37, Thomas Monjalon wrote:
> 2014-10-22 09:10, Marc Sune:
>> Fix for compilation warning 'missing-field-initializers' for some
>> GCC and clang versions introduced in commit 0c6bc8e
>>
>> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
> It's not needed to initialize all fields.
> This should be sufficient:
> +static struct rte_kni_memzone_pool kni_memzone_pool = {.initialized = 0};
>

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Thomas Monjalon]

2014-10-22 10:42, Marc Sune:
> The mutex needs to be initialized to RTE_SPINLOCK_INITIALIZER(0) too, or 
> move the initialization of the mutex to rte_kni_init().

RTE_SPINLOCK_INITIALIZER is { 0 }
By initializing one field, all other fields are set to 0, so spinlock also.
Just choose one field and it's OK.
It should be tested with ICC also but I think it's OK.

> I can prepare a second patch with one or the other option, if you want.

Yes please.

> On 22/10/14 10:37, Thomas Monjalon wrote:
> > 2014-10-22 09:10, Marc Sune:
> >> Fix for compilation warning 'missing-field-initializers' for some
> >> GCC and clang versions introduced in commit 0c6bc8e
> >>
> >> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
> > It's not needed to initialize all fields.
> > This should be sufficient:
> > +static struct rte_kni_memzone_pool kni_memzone_pool = {.initialized = 0};

Please Marc, don't top post.
Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Marc Sune]

On 22/10/14 10:50, Thomas Monjalon wrote:
> 2014-10-22 10:42, Marc Sune:
>> The mutex needs to be initialized to RTE_SPINLOCK_INITIALIZER(0) too, or
>> move the initialization of the mutex to rte_kni_init().
> RTE_SPINLOCK_INITIALIZER is { 0 }
> By initializing one field, all other fields are set to 0, so spinlock also.
> Just choose one field and it's OK.
> It should be tested with ICC also but I think it's OK.

Seems that you are right, at least for C99:

    C99 Standard 6.7.8.21

         If there are fewer initializers in a brace-enclosed list than
    there are elements or members of an aggregate, or fewer characters
    in a string literal used to initialize an array of known size than
    there are elements in the array, the remainder of the aggregate
    shall be initialized implicitly the same as objects that have static
    storage duration.


I am not sure if there can be problems with other C dialects (e.g. C11), 
I don't have the std here. So to prevent any problem with them (could 
produce a dead-lock during first rte_kni_alloc() that could be difficult 
to troubleshoot), I would still explicitly initialize the mutex, in one 
or the other way.

Just tell me if you agree and which one you prefer.

I don't have an ICC license. I am always trying it with GCC and clang.

Marc

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marc Sune
> Sent: Wednesday, October 22, 2014 10:50 AM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-
> initializers'
> 
> On 22/10/14 10:50, Thomas Monjalon wrote:
> > 2014-10-22 10:42, Marc Sune:
> >> The mutex needs to be initialized to RTE_SPINLOCK_INITIALIZER(0) too, or
> >> move the initialization of the mutex to rte_kni_init().
> > RTE_SPINLOCK_INITIALIZER is { 0 }
> > By initializing one field, all other fields are set to 0, so spinlock also.
> > Just choose one field and it's OK.
> > It should be tested with ICC also but I think it's OK.
> 
> Seems that you are right, at least for C99:
> 
>     C99 Standard 6.7.8.21
> 
>          If there are fewer initializers in a brace-enclosed list than
>     there are elements or members of an aggregate, or fewer characters
>     in a string literal used to initialize an array of known size than
>     there are elements in the array, the remainder of the aggregate
>     shall be initialized implicitly the same as objects that have static
>     storage duration.
> 
> 
> I am not sure if there can be problems with other C dialects (e.g. C11),
> I don't have the std here. So to prevent any problem with them (could
> produce a dead-lock during first rte_kni_alloc() that could be difficult
> to troubleshoot), I would still explicitly initialize the mutex, in one
> or the other way.
> 
> Just tell me if you agree and which one you prefer.
> 
> I don't have an ICC license. I am always trying it with GCC and clang.
> 
> Marc

ICC should be fine with this, it handles just initializing a single member of a structure as described by Thomas above.

/Bruce

Re: [dpdk-dev] [PATCH] KNI: fix compilation warning 'missing-field-initializers'

[Thomas Monjalon]

2014-10-22 11:49, Marc Sune:
> On 22/10/14 10:50, Thomas Monjalon wrote:
> > 2014-10-22 10:42, Marc Sune:
> >> The mutex needs to be initialized to RTE_SPINLOCK_INITIALIZER(0) too, or
> >> move the initialization of the mutex to rte_kni_init().
> > RTE_SPINLOCK_INITIALIZER is { 0 }
> > By initializing one field, all other fields are set to 0, so spinlock also.
> > Just choose one field and it's OK.
> > It should be tested with ICC also but I think it's OK.
> 
> Seems that you are right, at least for C99:
> 
>     C99 Standard 6.7.8.21
> 
>          If there are fewer initializers in a brace-enclosed list than
>     there are elements or members of an aggregate, or fewer characters
>     in a string literal used to initialize an array of known size than
>     there are elements in the array, the remainder of the aggregate
>     shall be initialized implicitly the same as objects that have static
>     storage duration.
> 
> 
> I am not sure if there can be problems with other C dialects (e.g. C11), 
> I don't have the std here. So to prevent any problem with them (could 
> produce a dead-lock during first rte_kni_alloc() that could be difficult 
> to troubleshoot), I would still explicitly initialize the mutex, in one 
> or the other way.
> 
> Just tell me if you agree and which one you prefer.

No problem for initializing mutex.

> I don't have an ICC license. I am always trying it with GCC and clang.

That's why it's the Intel job to support ICC in DPDK :)

-- 
Thomas

[dpdk-dev] [PATCH v2] KNI: fix compilation warning 'missing-field-initializers'

[Marc Sune]

Fix compilation warning 'missing-field-initializers' for some GCC and clang
versions introduced in commit 0c6bc8e due to the use of C89/C90 initializers.
Using C99-style initializers

Signed-off-by: Marc Sune <marc.sune@bisdn.de>
---
 lib/librte_kni/rte_kni.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index f64a0a8..ab5ca38 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -131,7 +131,9 @@ static void kni_free_mbufs(struct rte_kni *kni);
 static void kni_allocate_mbufs(struct rte_kni *kni);
 
 static volatile int kni_fd = -1;
-static struct rte_kni_memzone_pool kni_memzone_pool = {0};
+static struct rte_kni_memzone_pool kni_memzone_pool = {
+	.initialized = 0,
+};
 
 static const struct rte_memzone *
 kni_memzone_reserve(const char *name, size_t len, int socket_id,
@@ -224,6 +226,7 @@ rte_kni_init(unsigned int max_kni_ifaces)
 	kni_memzone_pool.initialized = 1;
 	kni_memzone_pool.max_ifaces = max_kni_ifaces;
 	kni_memzone_pool.free = &kni_memzone_pool.slots[0];
+	rte_spinlock_init(&kni_memzone_pool.mutex);
 
 	/* Pre-allocate all memzones of all the slots; panic on error */
 	for (i = 0; i < max_kni_ifaces; i++) {
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH v2] KNI: fix compilation warning 'missing-field-initializers'

[Thomas Monjalon]

2014-10-22 12:23, Marc Sune:
> Fix compilation warning 'missing-field-initializers' for some GCC and clang
> versions introduced in commit 0c6bc8e due to the use of C89/C90 initializers.
> Using C99-style initializers
> 
> Signed-off-by: Marc Sune <marc.sune@bisdn.de>

Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Applied

Thanks
-- 
Thomas