Re: [dpdk-dev] [RFC v9] /net: memory interface (memif)

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 5/20/2019 11:18 AM, Jakub Grajciar wrote:
> Memory interface (memif), provides high performance
> packet transfer over shared memory.

Can you please update the patch title as following in next version:
net/memif: introduce memory interface (memif) PMD

Can you please drop the RFC from next version, the patch is mature enough and we
can start regular versioning on it (without RFC), and RFC tag prefent DPDK
community lab to process the patch, so dropping the RFC will help there.

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

> +
> +.. csv-table:: **Memif configuration options**
> +   :header: "Option", "Description", "Default", "Valid value"
> +
> +   "id=0", "Used to identify peer interface", "0", "uint32_t"
> +   "role=master", "Set memif role", "slave", "master|slave"
> +   "bsize=1024", "Size of single packet buffer", "2048", "uint16_t"

What happens is 'bsize < mbuf size'? I didn't see any check in the code but is
there any assumption around this?
Or any assumption that slave and master packet should be same? Or any other
relation?
If there is any assumption it may be good to add checks to the code and document
here.

> +   "rsize=11", "Log2 of ring size. If rsize is 10, actual ring size is 1024", "10", "1-14"
> +   "nrxq=2", "Number of RX queues", "1", "255"
> +   "ntxq=2", "Number of TX queues", "1", "255"
> +   "socket=/tmp/memif.sock", "Socket filename", "/tmp/memif.sock", "string len 256"
> +   "mac=01:23:45:ab:cd:ef", "Mac address", "01:ab:23:cd:45:ef", ""
> +   "secret=abc123", "Secret is an optional security option, which if specified, must be matched by peer", "", "string len 24"
> +   "zero-copy=yes", "Enable/disable zero-copy slave mode", "no", "yes|no"
> +
<...>

> +Example: testpmd and testpmd
> +----------------------------
> +In this example we run two instances of testpmd application and transmit packets over memif.
> +
> +First create ``master`` interface::
> +
> +    #./testpmd -l 0-1 --proc-type=primary --file-prefix=pmd1 --vdev=net_memif,role=master -- -i
> +
> +Now create ``slave`` interface (master must be already running so the slave will connect)::
> +
> +    #./testpmd -l 2-3 --proc-type=primary --file-prefix=pmd2 --vdev=net_memif -- -i
> +
> +Set forwarding mode in one of the instances to 'rx only' and the other to 'tx only'::
> +
> +    testpmd> set fwd rxonly
> +    testpmd> start
> +
> +    testpmd> set fwd txonly
> +    testpmd> start

Also following works, above sample was not clear for me if the shared buffer is
only for one direction communication, but it is not the case, you can consider
adding below as well to clarify this:

Slave:
    testpmd> start

Master:
    testpmd> start tx_first


And if you want you can add multiple queue sample too, which gives better
performance:

master:
./testpmd -l 0-1 --proc-type=primary --file-prefix=pmd1
--vdev=net_memif,role=master -- -i --rxq=4 --txq=4

slave:
./testpmd -l 2-3 --proc-type=primary --file-prefix=pmd2
--vdev=net_memif,role=slave -- -i --rxq=4 --txq=4

<...>

> +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool
> +LDLIBS += -lrte_ethdev -lrte_kvargs
> +LDLIBS += -lrte_hash

Requires to be linked against vdev bus library for shared build:
LDLIBS += -lrte_bus_vdev

Can you please be sure to that PMD can be compiled as shared library?

<...>

> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2018-2019 Cisco Systems, Inc.  All rights reserved.
> +
> +if host_machine.system() != 'linux'
> +        build = false
> +endif
> +
> +sources = files('rte_eth_memif.c',
> +		'memif_socket.c')
> +
> +allow_experimental_apis = true

Can you please list the used experimental APIs here in a commented way, as it is
done in Makefile?

<...>

> +static void
> +memif_dev_close(struct rte_eth_dev *dev)
> +{
> +	struct pmd_internals *pmd = dev->data->dev_private;
> +
> +	memif_msg_enq_disconnect(pmd->cc, "Device closed", 0);
> +	memif_disconnect(dev);
> +
> +	memif_socket_remove_device(dev);

On latest agreement, .dev_close should free all internal data structures, and
since 'RTE_ETH_DEV_CLOSE_REMOVE' flag is set, generic ones will be cleaned by API.
Please check what is clean by API from 'rte_eth_dev_release_port()' remaining
memmory allocated by PMD should be freed here (rx, tx queues for example)

<...>

> +	/* TX stats */
> +	for (i = 0; i < nq; i++) {
> +		mq = dev->data->tx_queues[i];
> +		stats->q_opackets[i] = mq->n_pkts;
> +		stats->q_obytes[i] = mq->n_bytes;
> +		stats->q_errors[i] = mq->n_err;

'q_errors' is for Rx, there is a patchset on top of the next-net head, right now
this value only incorporate to overall Tx error stats (oerrors) not in this field.

<...>

> +
> +	eth_dev->data->dev_flags &= RTE_ETH_DEV_CLOSE_REMOVE;

Are you sure you want to '&' this flag, shouldn't it be "|="?

<...>

> +/* check if directory exists and if we have permission to read/write */
> +static int
> +memif_check_socket_filename(const char *filename)
> +{
> +	char *dir = NULL, *tmp;
> +	uint32_t idx;
> +	int ret = 0;
> +
> +	tmp = strrchr(filename, '/');
> +	if (tmp != NULL) {
> +		idx = tmp - filename;
> +		dir = rte_zmalloc("memif_tmp", sizeof(char) * (idx + 1), 0);
> +		if (dir == NULL) {
> +			MIF_LOG(ERR, "Failed to allocate memory.");
> +			return -1;
> +		}
> +		strlcpy(dir, filename, sizeof(char) * (idx + 1));
> +	}
> +
> +	if (dir == NULL || (faccessat(-1, dir, F_OK | R_OK |
> +					W_OK, AT_EACCESS) < 0)) {
> +		MIF_LOG(ERR, "Invalid directory: '%s'.", dir);

Getting following build error from "gcc (GCC) 9.1.1 20190503 (Red Hat 9.1.1-1)":

In file included from .../dpdk/drivers/net/memif/rte_eth_memif.c:27:
In function ‘memif_check_socket_filename’,
    inlined from ‘memif_set_socket_filename’ at
.../dpdk/drivers/net/memif/rte_eth_memif.c:1052:9:
.../dpdk/drivers/net/memif/rte_eth_memif.h:35:2: error: ‘%s’ directive argument
is null [-Werror=format-overflow=]
   35 |  rte_log(RTE_LOG_ ## level, memif_logtype, \
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   36 |   "%s(): " fmt "\n", __func__, ##args)
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.../dpdk/drivers/net/memif/rte_eth_memif.c:1035:3: note: in expansion of macro
‘MIF_LOG’
 1035 |   MIF_LOG(ERR, "Invalid directory: '%s'.", dir);
      |   ^~~~~~~
.../dpdk/drivers/net/memif/rte_eth_memif.c: In function ‘memif_set_socket_filename’:
.../dpdk/drivers/net/memif/rte_eth_memif.c:1098:37: note: format string is
defined here
 1098 |  MIF_LOG(INFO, "Initialize MEMIF: %s.", name);


<...>

> +static int
> +memif_set_mac(const char *key __rte_unused, const char *value, void *extra_args)
> +{
> +	struct ether_addr *ether_addr = (struct ether_addr *)extra_args;

Can you please rebase next version on top of latest head, which got some patches
that are adding rte_ prefix to these structs.

<...>

> +#ifndef _RTE_ETH_MEMIF_H_
> +#define _RTE_ETH_MEMIF_H_
> +
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif				/* GNU_SOURCE */

Why this was required?