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 <jgraj...@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?