[dpdk-dev] [RFC PATCH] ivshmem ring aliases

[dpdk-dev] [RFC PATCH] ivshmem ring aliases

[David Verbeiren]

The goal of this parch is to allow VMs to use standard ring names regardless of the names
given to the rings by host environment. It applies to configurations using ivshmem.

With shared memory rings, all VMs share a single namespace for the rings. However, a VM
will typically expect to find its rings with a pre-determined name (e.g. p1_rx, p1_tx)
regardless of how it's deployed, inserted in a service chain, or of which other VMs are
deployed alongside it. Hence, it is desirable to introduce a level of indirection where
the host can set a mapping from the actual ring names (e.g. dpdkr0_rx|tx with OVS) and
the names that will be visible in the VM. This patch provides a simple implementation
of such a mapping scheme.

Since the mapping must be VM specific, the aliases are inserted into the IVSHMEM metadata
area by the host and the guest side uses thoses aliases when doing rte_ring_lookup().

A new function, rte_ivshmem_add_ring_alias() is provided in librte_ivshmem to populate
alias entries in the host environment when creating the per-VM metadata.

Signed-off-by: David Verbeiren <david.verbeiren@gmail.com>
---
 config/defconfig_x86_64-ivshmem-linuxapp-gcc |  1 +
 lib/librte_eal/common/include/rte_eal.h      | 12 ++++++++
 lib/librte_eal/linuxapp/eal/eal_ivshmem.c    | 38 +++++++++++++++++++++++++
 lib/librte_ivshmem/rte_ivshmem.c             | 42 ++++++++++++++++++++++++++++
 lib/librte_ivshmem/rte_ivshmem.h             | 22 +++++++++++++++
 lib/librte_ring/rte_ring.c                   |  6 ++++
 6 files changed, 121 insertions(+)

diff --git a/config/defconfig_x86_64-ivshmem-linuxapp-gcc b/config/defconfig_x86_64-ivshmem-linuxapp-gcc
index 41ac5c3..2dc7674 100644
--- a/config/defconfig_x86_64-ivshmem-linuxapp-gcc
+++ b/config/defconfig_x86_64-ivshmem-linuxapp-gcc
@@ -44,6 +44,7 @@ CONFIG_RTE_LIBRTE_IVSHMEM_DEBUG=n
 CONFIG_RTE_LIBRTE_IVSHMEM_MAX_PCI_DEVS=4
 CONFIG_RTE_LIBRTE_IVSHMEM_MAX_ENTRIES=128
 CONFIG_RTE_LIBRTE_IVSHMEM_MAX_METADATA_FILES=32
+CONFIG_RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS=64
 
 # Set EAL to single file segments
 CONFIG_RTE_EAL_SINGLE_FILE_SEGMENTS=y
\ No newline at end of file
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 0e99c31..02aea8e 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -234,6 +234,18 @@ static inline int rte_gettid(void)
 	return RTE_PER_LCORE(_thread_id);
 }
 
+/**
+ * Perform a lookup in the IVSHMEM ring aliases
+ *
+ * @param alias
+ *   Ring alias name to search for.
+ * @return
+ *   On success, returns the actual ring name corresponding to the
+ *   provided alias.
+ *   Returns NULL if the alias is not known.
+ */
+const char * rte_eal_ivshmem_alias_get(const char * alias);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
index 28ddf09..4989c5c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
+++ b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
@@ -94,6 +94,7 @@ struct ivshmem_shared_config {
 	uint32_t segment_idx;
 	struct ivshmem_pci_device pci_devs[RTE_LIBRTE_IVSHMEM_MAX_PCI_DEVS];
 	uint32_t pci_devs_idx;
+	struct rte_ivshmem_ring_alias ring_alias[RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS];
 };
 static struct ivshmem_shared_config * ivshmem_config;
 static int memseg_idx;
@@ -369,6 +370,26 @@ read_metadata(char * path, int path_len, int fd, uint64_t flen)
 	}
 	ivshmem_config->segment_idx = idx;
 
+	int j = 0;  /* supports aliases from multiple IVSHMEM devices */ 
+	for (i = 0; i < RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS; i++) {
+		if (metadata.ring_alias[i].ring_alias[0] == '\0')
+			break;
+
+		RTE_LOG(DEBUG, EAL, "Alias[%d]: %s -> %s\n", i, metadata.ring_alias[i].ring_alias,
+				metadata.ring_alias[i].ring_name);
+		for (; j < RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS; j++) {
+			if (ivshmem_config->ring_alias[j].ring_alias[0] == '\0')
+				break;
+		}
+		if (j >= RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS) {
+			RTE_LOG(ERR, EAL, "Not enough space for alias (max %d)!\n", RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS);
+			return -1;
+		}
+
+		strncpy(ivshmem_config->ring_alias[j].ring_alias, metadata.ring_alias[i].ring_alias, RTE_RING_NAMESIZE);
+		strncpy(ivshmem_config->ring_alias[j].ring_name, metadata.ring_alias[i].ring_name, RTE_RING_NAMESIZE);
+	}
+
 	return 0;
 }
 
@@ -735,6 +756,23 @@ map_all_segments(void)
 	return 0;
 }
 
+const char * rte_eal_ivshmem_alias_get(const char* alias)
+{
+	if (ivshmem_config == NULL)
+		return NULL;
+
+	unsigned i;
+	for (i = 0; i < RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS; i++) {
+		if (strncmp(ivshmem_config->ring_alias[i].ring_alias, alias, RTE_RING_NAMESIZE) == 0)
+			break;
+	}
+
+	if (i >= RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS)
+		return NULL;
+
+	return ivshmem_config->ring_alias[i].ring_name;
+}
+
 /* this happens at a later stage, after general EAL memory initialization */
 int
 rte_eal_ivshmem_obj_init(void)
diff --git a/lib/librte_ivshmem/rte_ivshmem.c b/lib/librte_ivshmem/rte_ivshmem.c
index 8fc4b57..8eba074 100644
--- a/lib/librte_ivshmem/rte_ivshmem.c
+++ b/lib/librte_ivshmem/rte_ivshmem.c
@@ -566,6 +566,48 @@ add_mempool_to_metadata(const struct rte_mempool * mp,
 	return add_ring_to_metadata(mp->ring, config);
 }
 
+int rte_ivshmem_add_ring_alias(const struct rte_ring * r, const char * alias, const char * name)
+{
+	struct ivshmem_config * config;
+
+	if (name == NULL || r == NULL || alias == NULL || *alias == '\0')
+		return -1;
+
+	config = get_config_by_name(name);
+
+	if (config == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot find IVSHMEM config %s!\n", name);
+		return -1;
+	}
+
+	struct rte_ivshmem_metadata * metadata = config->metadata;
+
+	rte_spinlock_lock(&config->sl);
+
+	/* Find free slot */
+	unsigned i;
+	for (i = 0; i < RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS; i++) {
+		if (metadata->ring_alias[i].ring_alias[0] == '\0')
+			break;
+	}
+
+	if (i >= RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS) {
+		RTE_LOG(ERR, EAL, "IVSHMEM ring alias space is full!\n");
+		goto fail;
+	}
+
+	RTE_LOG(DEBUG, EAL, "Adding alias '%s'->'%s' to metadata %s\n",
+			alias, r->name, metadata->name);
+	strncpy(metadata->ring_alias[i].ring_alias, alias, RTE_RING_NAMESIZE);
+	strncpy(metadata->ring_alias[i].ring_name, r->name, RTE_RING_NAMESIZE);
+
+	rte_spinlock_unlock(&config->sl);
+	return 0;
+fail:
+	rte_spinlock_unlock(&config->sl);
+	return -1;
+}
+
 int
 rte_ivshmem_metadata_add_ring(const struct rte_ring * r, const char * name)
 {
diff --git a/lib/librte_ivshmem/rte_ivshmem.h b/lib/librte_ivshmem/rte_ivshmem.h
index a5d36d6..1ea65f5 100644
--- a/lib/librte_ivshmem/rte_ivshmem.h
+++ b/lib/librte_ivshmem/rte_ivshmem.h
@@ -60,6 +60,11 @@ struct rte_ivshmem_metadata_entry {
 	uint64_t offset;	/**< offset of memzone within IVSHMEM device */
 };
 
+struct rte_ivshmem_ring_alias {
+	char ring_alias[RTE_RING_NAMESIZE];
+	char ring_name[RTE_RING_NAMESIZE];
+};
+
 /**
  * Structure that holds IVSHMEM metadata.
  */
@@ -68,6 +73,7 @@ struct rte_ivshmem_metadata {
 	char name[IVSHMEM_NAME_LEN];	/**< name of the metadata file */
 	struct rte_ivshmem_metadata_entry entry[RTE_LIBRTE_IVSHMEM_MAX_ENTRIES];
 			/**< metadata entries */
+	struct rte_ivshmem_ring_alias ring_alias[RTE_LIBRTE_IVSHMEM_MAX_RING_ALIAS];
 };
 
 /**
@@ -113,6 +119,22 @@ int rte_ivshmem_metadata_add_ring(const struct rte_ring * r,
 		const char * md_name);
 
 /**
+ * Adds a ring alias to a specific metadata file
+ *
+ * @param r
+ *  Ring descriptor for which an alias is to be added
+ * @param alias
+ *  Name under which the ring will be available in the guest
+ * @param md_name
+ *  Name of metadata file for the ring to be added to
+ *
+ * @return
+ *  - On success, zero
+ *  - On failure, a negative value
+ */
+int rte_ivshmem_add_ring_alias(const struct rte_ring * r, const char * alias, const char * md_name);
+
+/**
  * Adds a mempool to a specific metadata file
  *
  * @param mp
diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c
index d80faf3..12efb8f 100644
--- a/lib/librte_ring/rte_ring.c
+++ b/lib/librte_ring/rte_ring.c
@@ -352,6 +352,12 @@ rte_ring_lookup(const char *name)
 	struct rte_ring *r = NULL;
 	struct rte_ring_list *ring_list;
 
+#ifdef RTE_LIBRTE_IVSHMEM
+	const char * target_name  = rte_eal_ivshmem_alias_get(name);
+	if (target_name)
+		name = target_name;
+#endif
+
 	ring_list = RTE_TAILQ_CAST(rte_ring_tailq.head, rte_ring_list);
 
 	rte_rwlock_read_lock(RTE_EAL_TAILQ_RWLOCK);
-- 
1.9.1

Re: [dpdk-dev] [RFC PATCH] ivshmem ring aliases

[Burakov, Anatoly]

> The goal of this parch is to allow VMs to use standard ring names regardless
> of the names given to the rings by host environment. It applies to
> configurations using ivshmem.
> 
> With shared memory rings, all VMs share a single namespace for the rings.
> However, a VM will typically expect to find its rings with a pre-determined
> name (e.g. p1_rx, p1_tx) regardless of how it's deployed, inserted in a
> service chain, or of which other VMs are deployed alongside it. Hence, it is
> desirable to introduce a level of indirection where the host can set a mapping
> from the actual ring names (e.g. dpdkr0_rx|tx with OVS) and the names that
> will be visible in the VM. This patch provides a simple implementation of such
> a mapping scheme.
> 
> Since the mapping must be VM specific, the aliases are inserted into the
> IVSHMEM metadata area by the host and the guest side uses thoses aliases
> when doing rte_ring_lookup().
> 
> A new function, rte_ivshmem_add_ring_alias() is provided in
> librte_ivshmem to populate alias entries in the host environment when
> creating the per-VM metadata.

Don't have any objections to this RFC, looks sensible to me as a concept. So, provided the tests are passing,

Acked-by: Anatoly  Burakov <anatoly.burakov@intel.com>


 

Re: [dpdk-dev] [RFC PATCH] ivshmem ring aliases

[Thomas Monjalon]

2016-02-24 11:33, David Verbeiren:
> The goal of this parch is to allow VMs to use standard ring names regardless of the names
> given to the rings by host environment. It applies to configurations using ivshmem.
> 
> With shared memory rings, all VMs share a single namespace for the rings. However, a VM
> will typically expect to find its rings with a pre-determined name (e.g. p1_rx, p1_tx)
> regardless of how it's deployed, inserted in a service chain, or of which other VMs are
> deployed alongside it. Hence, it is desirable to introduce a level of indirection where
> the host can set a mapping from the actual ring names (e.g. dpdkr0_rx|tx with OVS) and
> the names that will be visible in the VM. This patch provides a simple implementation
> of such a mapping scheme.
> 
> Since the mapping must be VM specific, the aliases are inserted into the IVSHMEM metadata
> area by the host and the guest side uses thoses aliases when doing rte_ring_lookup().
> 
> A new function, rte_ivshmem_add_ring_alias() is provided in librte_ivshmem to populate
> alias entries in the host environment when creating the per-VM metadata.

I'm still not sure this library is a good idea at all.
This patch continue the tradition of librte_ivshmem by adding more
#ifdef in the code (in rte_ring here).
We could also comment the compile time values or the checkpatch warnings.
But more importantly, what is the use case of this library and why is
it important to have such support in DPDK?

> --- a/lib/librte_ring/rte_ring.c
> +++ b/lib/librte_ring/rte_ring.c
> @@ -352,6 +352,12 @@ rte_ring_lookup(const char *name)
>  	struct rte_ring *r = NULL;
>  	struct rte_ring_list *ring_list;
>  
> +#ifdef RTE_LIBRTE_IVSHMEM
> +	const char * target_name  = rte_eal_ivshmem_alias_get(name);
> +	if (target_name)
> +		name = target_name;
> +#endif

This #ifdef should not exist.