Skip to contentSkip to conversation

DPDK 1.6.1 fixes

PATCH In review Stephen Hemminger 2 participants 13 messages Mar 7, 2014 – Apr 17, 2014 6 messages not in the archive

lore
Share & tools
lore
#0 lore raw

cover DPDK 1.6.1 fixes

These are patches for issues found during integration of 1.6.1 and also during product development on earlier release.

#1 lore raw

PATCH 1/6 rte_mbuf: copy offload flags when doing attach/clone of mbuf

rte_pktmbuf_attach copies the packet meta data but does not copy the offload flags. This means that cloned packets lose their offload settings such as vlan tag.

lib/librte_mbuf/rte_mbuf.h +1 -0
@@ -624,6 +624,7 @@ static inline void rte_pktmbuf_attach(st
 	mi->pkt.next = NULL;
 	mi->pkt.pkt_len = mi->pkt.data_len;
 	mi->pkt.nb_segs = 1;
+	mi->ol_flags = md->ol_flags;
 
 	__rte_mbuf_sanity_check(mi, RTE_MBUF_PKT, 1);
 	__rte_mbuf_sanity_check(md, RTE_MBUF_PKT, 0);
#4 lore raw

PATCH 2/6 qos: use rte_zmalloc instead of memzone for allocation

In reply to Stephen Hemminger

Memory zone's are inflexible and can not be destroyed. The size is fixed when initially created therefor QoS parameters could not be modified at run time, because table size for a subport might change.

lib/librte_sched/rte_sched.c +6 -18
@@ -37,7 +37,7 @@
 #include <rte_common.h>
 #include <rte_log.h>
 #include <rte_memory.h>
-#include <rte_memzone.h>
+#include <rte_malloc.h>
 #include <rte_cycles.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
@@ -613,7 +613,6 @@ struct rte_sched_port *
 rte_sched_port_config(struct rte_sched_port_params *params)
 {
 	struct rte_sched_port *port = NULL;
-	const struct rte_memzone *mz = NULL;
 	uint32_t mem_size, bmp_mem_size, n_queues_per_port, i;
 	
 	/* Check user parameters. Determine the amount of memory to allocate */
@@ -623,21 +622,10 @@ rte_sched_port_config(struct rte_sched_p
 	}
 	
 	/* Allocate memory to store the data structures */
-	mz = rte_memzone_lookup(params->name);
-	if (mz) {
-		/* Use existing memzone, provided that its size is big enough */
-		if (mz->len < mem_size) {
-			return NULL;
-		}
-	} else {
-		/* Create new memzone */
-		mz = rte_memzone_reserve(params->name, mem_size, params->socket, 0);		
-		if (mz == NULL) {
-			return NULL;
-		}
+	port = rte_zmalloc("qos_params", mem_size, CACHE_LINE_SIZE);
+	if (port == NULL) {
+		return NULL;
 	}
-	memset(mz->addr, 0, mem_size);
-	port = (struct rte_sched_port *) mz->addr;
 
 	/* User parameters */
 	port->n_subports_per_port = params->n_subports_per_port;
@@ -716,9 +704,9 @@ rte_sched_port_free(struct rte_sched_por
 	if (port == NULL){
 		return;
 	}
+
 	rte_bitmap_free(port->bmp);
-	
-	return;
+	rte_free(port);
 }
 
 static void
#7 lore raw

PATCH 3/6 rte_jhash: make arg to rte_jhash2 const

In reply to Stephen Hemminger

The argument to rte_jhash2() is not changed.

lib/librte_hash/rte_jhash.h +1 -1
index e137d13..ae91bf7 100644
@@ -149,7 +149,7 @@ rte_jhash(const void *key, uint32_t length, uint32_t initval)
  *   Calculated hash value.
  */
 static inline uint32_t
-rte_jhash2(uint32_t *k, uint32_t length, uint32_t initval)
+rte_jhash2(const uint32_t *k, uint32_t length, uint32_t initval)
 {
 	uint32_t a, b, c, len;
 
#10 lore raw

PATCH 4/6 mempool: use GCC push/pop

In reply to Stephen Hemminger

The include file should not change the GCC compile options for the whole file being compiled, but only for the one inline function that needs it. Using the push_options/pop_options fixes this.

lib/librte_mempool/rte_mempool.h +2 -1
@@ -314,6 +314,7 @@ static inline void __mempool_write_trail
  */
 #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
 #ifndef __INTEL_COMPILER
+#pragma GCC push_options
 #pragma GCC diagnostic ignored "-Wcast-qual"
 #endif
 static inline void __mempool_check_cookies(const struct rte_mempool *mp,
@@ -380,7 +381,7 @@ static inline void __mempool_check_cooki
 	}
 }
 #ifndef __INTEL_COMPILER
-#pragma GCC diagnostic error "-Wcast-qual"
+#pragma GCC pop_options
 #endif
 #else
 #define __mempool_check_cookies(mp, obj_table_const, n, free) do {} while(0)
#13 lore raw

PATCH 5/6 xen: dont create dependency on cmdline library

In reply to Stephen Hemminger

The driver should not introduce an unnecessary dependency on the cmdline code. We don't build that code since it is not used in our product.

---
 lib/librte_pmd_xenvirt/rte_eth_xenvirt.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
lib/librte_pmd_xenvirt/rte_eth_xenvirt.c +6 -6
index d844845..42a20ba 100644
@@ -53,8 +53,6 @@
 #include <rte_malloc.h>
 #include <rte_memcpy.h>
 #include <rte_string_fns.h>
-#include <cmdline_parse.h>
-#include <cmdline_parse_etheraddr.h>
 
 #include "rte_xen_lib.h"
 #include "virtqueue.h"
@@ -63,6 +61,9 @@
 #define VQ_DESC_NUM 256
 #define VIRTIO_MBUF_BURST_SZ 64
 
+/* defined in <netinet/ether.h> but that has conflicts with rte_ethdev.h */
+extern struct ether_addr *ether_aton(const char *);
+
 /* virtio_idx is increased after new device is created.*/
 static int virtio_idx = 0;
 
@@ -584,15 +585,14 @@ rte_eth_xenvirt_parse_args(struct xenvirt_dict *dict,
 
 		if (!strncmp(pair[0], RTE_ETH_XENVIRT_MAC_PARAM,
 				sizeof(RTE_ETH_XENVIRT_MAC_PARAM))) {
-			if (cmdline_parse_etheraddr(NULL,
-							pair[1],
-							&dict->addr) < 0) {
+			struct ether_addr *ea = ether_aton(pair[1]);
+			if (ea == NULL) {
 				RTE_LOG(ERR, PMD,
 					"Invalid %s device ether address\n",
 					name);
 				goto err;
 			}
-
+			ether_addr_copy(ea, &dict->addr);
 			dict->addr_valid = 1;
 		}
 	}
Signature · 1 line
1.7.10.4
#16 lore raw

PATCH 6/6 ivshmem: fix errors identified by hardening

In reply to Stephen Hemminger

Need to pass mode argument to open with O_CREAT.
Must check return value from ftruncate().

lib/librte_eal/linuxapp/eal/eal_ivshmem.c +5 -2
index 6191fef..5f64873 100644
@@ -472,7 +472,7 @@ create_shared_config(void)
 	rte_snprintf(path, sizeof(path), IVSHMEM_CONFIG_PATH,
 			internal_config.hugefile_prefix);
 
-	fd = open(path, O_CREAT | O_RDWR);
+	fd = open(path, O_CREAT | O_RDWR, 0600);
 
 	if (fd < 0) {
 		RTE_LOG(ERR, EAL, "Could not open %s: %s\n", path, strerror(errno));
@@ -486,7 +486,10 @@ create_shared_config(void)
 		return -1;
 	}
 
-	ftruncate(fd, sizeof(struct ivshmem_shared_config));
+	if (ftruncate(fd, sizeof(struct ivshmem_shared_config)) < 0) {
+		RTE_LOG(ERR, EAL, "ftruncate failed: %s\n", strerror(errno));
+		return -1;
+	}
 
 	ivshmem_config = mmap(NULL, sizeof(struct ivshmem_shared_config),
 			PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
#3 lore raw

Re: [PATCH 1/6] rte_mbuf: copy offload flags when doing attach/clone of mbuf

In reply to a message not in the archive

07/03/2014 10:13, Stephen Hemminger:

Quoted 5 lines

rte_pktmbuf_attach copies the packet meta data but does not copy the offload flags. This means that cloned packets lose their offload settings such as vlan tag.

Acked and applied for version 1.6.0r2.

Thank you

Signature · 1 line
Thomas
#12 lore raw

Re: [PATCH 4/6] mempool: use GCC push/pop

In reply to a message not in the archive

07/03/2014 10:13, Stephen Hemminger:

Quoted 5 lines

The include file should not change the GCC compile options for the whole file being compiled, but only for the one inline function that needs it. Using the push_options/pop_options fixes this.

Acked and applied for version 1.6.0r2.

Thank you

Signature · 1 line
Thomas
#6 lore raw

Re: [PATCH 2/6] qos: use rte_zmalloc instead of memzone for allocation

In reply to a message not in the archive

07/03/2014 10:13, Stephen Hemminger:

Quoted 6 lines

Memory zone's are inflexible and can not be destroyed. The size is fixed when initially created therefor QoS parameters could not be modified at run time, because table size for a subport might change.

This change would probably deserve a description of the use case. Please explain in the log and/or comments why you need to increase this size.

Signature · 1 line
Thomas
#15 lore raw

Re: [PATCH 5/6] xen: dont create dependency on cmdline library

In reply to a message not in the archive

07/03/2014 10:13, Stephen Hemminger:

Quoted 2 lines

The driver should not introduce an unnecessary dependency on the cmdline code. We don't build that code since it is not used in our product.

Your product is not the concern but I agree this dependency is unnecessary. I'd just remove the comment about your product.

lib/librte_pmd_xenvirt/rte_eth_xenvirt.c line 65

Quoted 2 lines
lib/librte_pmd_xenvirt/rte_eth_xenvirt.c +2 -0
+/* defined in <netinet/ether.h> but that has conflicts with rte_ethdev.h */
+extern struct ether_addr *ether_aton(const char *);

It seems we should fix this conflict first.
I think the problem is about libc redefinitions in rte_ether.h. Patches are welcome.

Signature · 1 line
Thomas