[dpdk-dev] [PATCH 1/2] net/memif: enable loopback

[dpdk-dev] [PATCH 1/2] net/memif: enable loopback

[Július Milan]

With this patch it is possible to connect 2 DPDK memifs into loopback,
i.e. when they have the same id and different roles, as for example:
  "--vdev=net_memif0,role=master,id=0"
  "--vdev=net_memif1,role=slave,id=0"

Signed-off-by: Július Milan <jmilan.dev@gmail.com>
---
 drivers/net/memif/memif_socket.c  | 30 ++++++++++++++----------------
 drivers/net/memif/rte_eth_memif.c |  1 +
 drivers/net/memif/rte_eth_memif.h |  1 +
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
index ad5e30b96..553f8b004 100644
--- a/drivers/net/memif/memif_socket.c
+++ b/drivers/net/memif/memif_socket.c
@@ -203,7 +203,7 @@ memif_msg_receive_init(struct memif_control_channel *cc, memif_msg_t *msg)
 		dev = elt->dev;
 		pmd = dev->data->dev_private;
 		if (((pmd->flags & ETH_MEMIF_FLAG_DISABLED) == 0) &&
-		    pmd->id == i->id) {
+		    (pmd->id == i->id) && (pmd->role == MEMIF_ROLE_MASTER)) {
 			/* assign control channel to device */
 			cc->dev = dev;
 			pmd->cc = cc;
@@ -528,6 +528,7 @@ memif_disconnect(struct rte_eth_dev *dev)
 	pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTING;
 	pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTED;
 
+	rte_spinlock_lock(&pmd->cc_lock);
 	if (pmd->cc != NULL) {
 		/* Clear control message queue (except disconnect message if any). */
 		for (elt = TAILQ_FIRST(&pmd->cc->msg_queue); elt != NULL; elt = next) {
@@ -570,6 +571,7 @@ memif_disconnect(struct rte_eth_dev *dev)
 					"Failed to unregister control channel callback.");
 		}
 	}
+	rte_spinlock_unlock(&pmd->cc_lock);
 
 	/* unconfig interrupts */
 	for (i = 0; i < pmd->cfg.num_s2m_rings; i++) {
@@ -612,7 +614,8 @@ memif_disconnect(struct rte_eth_dev *dev)
 	/* reset connection configuration */
 	memset(&pmd->run, 0, sizeof(pmd->run));
 
-	MIF_LOG(DEBUG, "Disconnected.");
+	MIF_LOG(DEBUG, "Disconnected, id: %d, role: %s.", pmd->id,
+		(pmd->role == MEMIF_ROLE_MASTER) ? "master" : "slave");
 }
 
 static int
@@ -642,8 +645,12 @@ memif_msg_receive(struct memif_control_channel *cc)
 
 	size = recvmsg(cc->intr_handle.fd, &mh, 0);
 	if (size != sizeof(memif_msg_t)) {
-		MIF_LOG(DEBUG, "Invalid message size.");
-		memif_msg_enq_disconnect(cc, "Invalid message size", 0);
+		MIF_LOG(DEBUG, "Invalid message size = %ld", size);
+		if (size > 0)
+			/* 0 means end-of-file, negative size means error,
+			 * don't send further disconnect message in such cases.
+			 */
+			memif_msg_enq_disconnect(cc, "Invalid message size", 0);
 		return -1;
 	}
 	MIF_LOG(DEBUG, "Received msg type: %u.", msg.type);
@@ -965,20 +972,11 @@ memif_socket_init(struct rte_eth_dev *dev, const char *socket_filename)
 	}
 	pmd->socket_filename = socket->filename;
 
-	if (socket->listener != 0 && pmd->role == MEMIF_ROLE_SLAVE) {
-		MIF_LOG(ERR, "Socket is a listener.");
-		return -1;
-	} else if ((socket->listener == 0) && (pmd->role == MEMIF_ROLE_MASTER)) {
-		MIF_LOG(ERR, "Socket is not a listener.");
-		return -1;
-	}
-
 	TAILQ_FOREACH(elt, &socket->dev_queue, next) {
 		tmp_pmd = elt->dev->data->dev_private;
-		if (tmp_pmd->id == pmd->id) {
-			MIF_LOG(ERR, "Memif device with id %d already "
-				"exists on socket %s",
-				pmd->id, socket->filename);
+		if (tmp_pmd->id == pmd->id && tmp_pmd->role == pmd->role) {
+			MIF_LOG(ERR, "Two interfaces with the same id (%d) can "
+				"not have the same role.", pmd->id);
 			return -1;
 		}
 	}
diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 27c0f0924..81d71c53a 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -1491,6 +1491,7 @@ memif_create(struct rte_vdev_device *vdev, enum memif_role_t role,
 	pmd->cfg.num_m2s_rings = 0;
 
 	pmd->cfg.pkt_buffer_size = pkt_buffer_size;
+	rte_spinlock_init(&pmd->cc_lock);
 
 	data = eth_dev->data;
 	data->dev_private = pmd;
diff --git a/drivers/net/memif/rte_eth_memif.h b/drivers/net/memif/rte_eth_memif.h
index 0d2566392..6f45b7072 100644
--- a/drivers/net/memif/rte_eth_memif.h
+++ b/drivers/net/memif/rte_eth_memif.h
@@ -94,6 +94,7 @@ struct pmd_internals {
 	char secret[ETH_MEMIF_SECRET_SIZE]; /**< secret (optional security parameter) */
 
 	struct memif_control_channel *cc;	/**< control channel */
+	rte_spinlock_t cc_lock;			/**< control channel lock */
 
 	/* remote info */
 	char remote_name[RTE_DEV_NAME_MAX_LEN];		/**< remote app name */
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/2] net/memif: enable loopback

[Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco)]

> -----Original Message-----
> From: Július Milan <jmilan.dev@gmail.com>
> Sent: Wednesday, February 19, 2020 9:19 AM
> To: dev@dpdk.org; Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco)
> <jgrajcia@cisco.com>
> Subject: [dpdk-dev][PATCH 1/2] net/memif: enable loopback
> 
> With this patch it is possible to connect 2 DPDK memifs into loopback, i.e.
> when they have the same id and different roles, as for example:
>   "--vdev=net_memif0,role=master,id=0"
>   "--vdev=net_memif1,role=slave,id=0"
> 
> Signed-off-by: Július Milan <jmilan.dev@gmail.com>

Reviewed-by: Jakub Grajciar <jgrajcia@cisco.com>

Re: [dpdk-dev] [PATCH 1/2] net/memif: enable loopback

[Ferruh Yigit]

On 2/19/2020 8:18 AM, Július Milan wrote:
> With this patch it is possible to connect 2 DPDK memifs into loopback,
> i.e. when they have the same id and different roles, as for example:
>   "--vdev=net_memif0,role=master,id=0"
>   "--vdev=net_memif1,role=slave,id=0"
> 
> Signed-off-by: Július Milan <jmilan.dev@gmail.com>
> ---
>  drivers/net/memif/memif_socket.c  | 30 ++++++++++++++----------------
>  drivers/net/memif/rte_eth_memif.c |  1 +
>  drivers/net/memif/rte_eth_memif.h |  1 +
>  3 files changed, 16 insertions(+), 16 deletions(-)

Hi Július,

Can you please document this new feature in the memif documentation, and it may
be good to give a sample testpmd commands like ones already exist in that doc?

Thanks,
ferruh

Re: [dpdk-dev] [PATCH 1/2] net/memif: enable loopback

[Ferruh Yigit]

On 2/19/2020 8:18 AM, Július Milan wrote:
> With this patch it is possible to connect 2 DPDK memifs into loopback,
> i.e. when they have the same id and different roles, as for example:
>   "--vdev=net_memif0,role=master,id=0"
>   "--vdev=net_memif1,role=slave,id=0"
> 
> Signed-off-by: Július Milan <jmilan.dev@gmail.com>

<...>

> @@ -642,8 +645,12 @@ memif_msg_receive(struct memif_control_channel *cc)
>  
>  	size = recvmsg(cc->intr_handle.fd, &mh, 0);
>  	if (size != sizeof(memif_msg_t)) {
> -		MIF_LOG(DEBUG, "Invalid message size.");
> -		memif_msg_enq_disconnect(cc, "Invalid message size", 0);
> +		MIF_LOG(DEBUG, "Invalid message size = %ld", size);

'size' type is 'size_t', can you please use correct format specifier '%zu' for it?
This will fix the 32-bit build.

Re: [dpdk-dev] [PATCH 1/2] net/memif: enable loopback

[Ferruh Yigit]

On 3/9/2020 12:09 PM, Július Milan wrote:
> On Thu, Feb 27, 2020 at 04:46:09PM +0000, Ferruh Yigit wrote:
>> On 2/19/2020 8:18 AM, Július Milan wrote:
>>> With this patch it is possible to connect 2 DPDK memifs into loopback,
>>> i.e. when they have the same id and different roles, as for example:
>>>   "--vdev=net_memif0,role=master,id=0"
>>>   "--vdev=net_memif1,role=slave,id=0"
>>>
>>> Signed-off-by: Július Milan <jmilan.dev@gmail.com>
>>> ---
>>>  drivers/net/memif/memif_socket.c  | 30 ++++++++++++++----------------
>>>  drivers/net/memif/rte_eth_memif.c |  1 +
>>>  drivers/net/memif/rte_eth_memif.h |  1 +
>>>  3 files changed, 16 insertions(+), 16 deletions(-)
>>
>> Hi Július,
>>
>> Can you please document this new feature in the memif documentation, and it may
>> be good to give a sample testpmd commands like ones already exist in that doc?
>>
>> Thanks,
>> ferruh
>>
> 
> Hi Ferruh
> 
> Sure, I will send a next patch containing changes to doc/,
> not sure about the best workflow in cases like this.
> 

A new version of the patchset with documentation changes would be good,
but I see you already send the doc patch separately, that is OK, I can squash it
to the commit that feature added while merging.

Thanks,
ferruh

Re: [dpdk-dev] [PATCH 1/2] net/memif: enable loopback

[Július Milan]

On Thu, Feb 27, 2020 at 04:46:09PM +0000, Ferruh Yigit wrote:
>On 2/19/2020 8:18 AM, Július Milan wrote:
>> With this patch it is possible to connect 2 DPDK memifs into loopback,
>> i.e. when they have the same id and different roles, as for example:
>>   "--vdev=net_memif0,role=master,id=0"
>>   "--vdev=net_memif1,role=slave,id=0"
>> 
>> Signed-off-by: Július Milan <jmilan.dev@gmail.com>
>> ---
>>  drivers/net/memif/memif_socket.c  | 30 ++++++++++++++----------------
>>  drivers/net/memif/rte_eth_memif.c |  1 +
>>  drivers/net/memif/rte_eth_memif.h |  1 +
>>  3 files changed, 16 insertions(+), 16 deletions(-)
>
>Hi Július,
>
>Can you please document this new feature in the memif documentation, and it may
>be good to give a sample testpmd commands like ones already exist in that doc?
>
>Thanks,
>ferruh
>

Hi Ferruh

Sure, I will send a next patch containing changes to doc/,
not sure about the best workflow in cases like this.

Thanks,
Julius